Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

[Backport] Fix nil-pointer on accessing condition in autoscaler #634

Closed
wants to merge 1 commit into from

Conversation

skonto
Copy link

@skonto skonto commented Jan 7, 2021

Backport of knative#9794. I am wondering since there are direct calls to the struct fields elsewhere if we are going to hit this in other areas of the code base in the future. For example here.

@openshift-ci-robot
Copy link

Hi @skonto. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 7, 2021
@skonto
Copy link
Author

skonto commented Jan 7, 2021

@mgencur @nak3 fyi.

@nak3 nak3 added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 7, 2021
@mgencur
Copy link

mgencur commented Jan 7, 2021

Good point about the other places where the same access is used. I can't say for sure but it looks likely it will happen.

@mgencur
Copy link

mgencur commented Jan 7, 2021

/lgtm

Still the checks need to pass before it will be merged.

@nak3
Copy link

nak3 commented Jan 7, 2021

/lgtm
/approve

The test is required so we don't need /hold.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2021
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mgencur, nak3, skonto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 7, 2021
@skonto
Copy link
Author

skonto commented Jan 7, 2021

@mgencur I am not sure If we want to go and fix every call but it seems that they are not protected as getting a specific condition may return a nil pointer. Same applies to other structures btw. Maybe the other calls never get a nil in any scenario. Let's see if this fixes the issue we see in our tests.

@mgencur
Copy link

mgencur commented Jan 7, 2021

@skonto right, I wouldn't be fixing the other occurrences either. That fix should probably go upstream first. We'll retest the current fix with soak tests with the new build and see if the issue is gone. I believe it will be.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@skonto
Copy link
Author

skonto commented Jan 7, 2021

/retest

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link

openshift-ci bot commented Jan 8, 2021

@skonto: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/4.6-e2e-aws-ocp-46 4db8d2e link /test 4.6-e2e-aws-ocp-46

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@mgencur
Copy link

mgencur commented Jan 8, 2021

Thanks @skonto . This one was replaced by #635 which also resolves the test issues.

@mgencur mgencur closed this Jan 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants