Conversation
| if len(podList.Items) < 1 { | ||
| klog.Errorf("list of pods is empty") | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Does this matter? If the list of pods is empty, status should be degraded right?
There was a problem hiding this comment.
I think it doesn't matter, but do we expect pods not to be running when at this part of the test?
There was a problem hiding this comment.
Well my thought was we go through this cycle, delete a pod, status isn't degraded yet, so start again, pod has not been recreated yet, could the status be degraded by this point? I am not sure, I would assume they are independent since they're different controllers controlling these actions
| clusterOperator := &configv1.ClusterOperator{} | ||
|
|
||
| if err := wait.PollImmediate(RetryShort, WaitShort, func() (bool, error) { | ||
| if err := wait.PollImmediate(RetryShort, 10*time.Minute, func() (bool, error) { |
There was a problem hiding this comment.
Commit description says waits for 3 minutes, which is WaitMedium, but it's actually waiting for 10 minutes, should we update one of these? I think 3 minutes to become available should be sufficient shouldn't it?
| if err := wait.PollImmediate(RetryShort, 10*time.Minute, func() (bool, error) { | |
| if err := wait.PollImmediate(RetryShort, WaitMedium, func() (bool, error) { |
There was a problem hiding this comment.
3 min is the minimum time the mao expects the pod to have been available https://github.com/openshift/machine-api-operator/blob/master/pkg/operator/sync.go#L28
5 min is the total the mao waits for the pod to rollout out https://github.com/openshift/machine-api-operator/blob/master/pkg/operator/sync.go#L30
10 min is where we set the bar in our tests to consider this a failure. I set 10 to account for scenarios where this is running in parallel with tests that are disrupting the status.
I'm ok to put any lower (>5m) if you prefer.
There was a problem hiding this comment.
Nope, that makes sense, happy to leave as 10 mins now commit description is updated
| if len(podList.Items) < 1 { | ||
| klog.Errorf("list of pods is empty") | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
I think it doesn't matter, but do we expect pods not to be running when at this part of the test?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexander-demichev The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
Names for funcs which run actions in loop should be prefixed with wait. So if the specific action need to be called within any other "eventually" func it can be name appriopriately e.g IsStatusAvailable. The timeout is increased so this can succeed for status transitions. E.g the mao will wait for a pod to be running for at least 3min before considering it available. https://github.com/openshift/machine-api-operator/blob/master/pkg/operator/sync.go#L28
enxebre
force-pushed
the
degraded-validation
branch
from
ce50f6c to
1e45141
Compare
|
/retest |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
@enxebre: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci-robot
added
lifecycle/rotten
|
@enxebre: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. DetailsIn response to this:
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. |
6 participants
The first commit introduces a validation for the machine-api clusterOperator going degraded.
The second commit renames IsStatusAvailable -> WaitForStatusAvailable and increase the waiting timeout. This is needed as we are testing status transitions now. Names for funcs which run actions in loop should be prefixed with wait.
So if the specific action need to be called within any other "eventually" func it can be name appriopriately e.g IsStatusAvailable.
The timeout is increased so this can succeed for status transitions. E.g the mao will wait for a pod to be running for at least 3min before considering it available.
https://github.com/openshift/machine-api-operator/blob/master/pkg/operator/sync.go#L28
This PR is to test openshift/machine-api-operator#651