New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1753778: report Available when registry is explicitly Removed #387
Conversation
@dmage: This pull request references Bugzilla bug 1753778, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
/test e2e-aws-operator |
1 similar comment
/test e2e-aws-operator |
0d5e7c3
to
5ac40d9
Compare
@@ -87,10 +87,18 @@ func (c *Controller) syncStatus(cr *imageregistryv1.Config, deploy *appsapi.Depl | |||
Message: "", | |||
Reason: "", | |||
} | |||
if deploy == nil { | |||
if cr.Spec.ManagementState == operatorapiv1.Unmanaged { | |||
operatorAvailable.Status = operatorapiv1.ConditionTrue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is Unmanaged doesn't it make sense to set this to UNKNOWN
instead of true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but we don't want to block upgrades, so we report that everything is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bparees but honestly we are lying to CVO, really we may have problems, especially after upgrades. If we have the Unknown
state, maybe it's better to use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmage Think of it this way - if not in the Managed
state, Available
means "Can the operator do what you ask it to do?" Unless we are not running, the answer is yes/True
.
Ex: if the management state is changed from Unmanaged
to Managed
, will the operator react as expected? Yes.
test/e2e/managementstate_test.go
Outdated
t.Fatal(err) | ||
} | ||
|
||
// TODO(dmage): wait for the resource to be observed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we planning to wait for this one before merging? This could generate some flake events, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we have retries below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we don't need the TODO right? Or I misunderstand you?
/retest |
/lgtm |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
/hold Known issue with 1.16 storage tests (waiting for kubelet/RHCOS skew to be reconciled). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty neat. I have left a couple of comments more for sanity check.
pkg/operator/status.go
Outdated
if e, ok := applyError.(permanentError); ok { | ||
operatorAvailable.Message = applyError.Error() | ||
operatorAvailable.Reason = e.Reason | ||
} else if removed { | ||
operatorAvailable.Status = operatorapiv1.ConditionTrue | ||
operatorAvailable.Message = "The deployment is removed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this Message something like down below when we read:
"The registry is removed"
test/e2e/managementstate_test.go
Outdated
t.Fatal(err) | ||
} | ||
|
||
// TODO(dmage): wait for the resource to be observed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we don't need the TODO right? Or I misunderstand you?
test/e2e/managementstate_test.go
Outdated
|
||
// TODO(dmage): wait for the resource to be observed | ||
|
||
err = client.Deployments(imageregistryv1.ImageRegistryOperatorNamespace).Delete(imageregistryv1.ImageRegistryName, &metav1.DeleteOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this Delete automatically happen slightly after we set the Management State to Removed?
/lgtm cancel @dmage please address or remove the TODOs in your comments. /retest |
/hold cancel RHCOS skew issues should be resolved now |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dmage, ricardomaraschini 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 |
@dmage: All pull requests linked via external trackers have merged. Bugzilla bug 1753778 has been moved to the MODIFIED state. In 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. |
/cherrypick release-4.2 |
@dmage: new pull request created: #412 In 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. |
/cherrypick release-4.1 |
@ricardomaraschini: #387 failed to apply on top of branch "release-4.1":
In 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. |
No description provided.