ARO-3258: propagate errors of ARO PullSecret controller to ARO operator #3947
Conversation
lbossis
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
SudoBrendan,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig,
tiguelu,
SrinivasAtmakuri,
mociarain,
kimorris27,
tsatam,
bitoku and
fahlmant
as code owners
|
@microsoft-github-policy-service agree company="Red Hat" |
| return reconcile.Result{}, err | ||
| } | ||
|
|
||
| // fix pull secret if its broken to have at least the ARO pull secret | ||
| userSecret, err = r.ensureGlobalPullSecret(ctx, operatorSecret, userSecret) | ||
| if err != nil { | ||
| r.Log.Error(err) |
There was a problem hiding this comment.
We probably want to SetDegraded here if we can't ensure the global pull secret. Here's an example: https://github.com/Azure/ARO-RP/pull/3177/files#diff-17756e345e334809c4f7ea3663119196d4de8e8085aca1b0da6a96e708d5d251R84
There was a problem hiding this comment.
Resolving as we've agreed in the JIRA to not set degraded as part of this PR as it could impact user's ability to rotate their pull secret.
There was a problem hiding this comment.
We can safely SetDegraded here without impacting any functionality. The controller will still continue to execute as it would normally, and the controller Degraded status will (as of right now) not be cascaded to the ARO Cluster Operator itself, so this controller being degraded will not e.g. block cluster upgrades.
pkg/operator/controllers/pullsecret/pullsecret_controller_test.go
Outdated
Show resolved
Hide resolved
| @@ -285,11 +287,11 @@ func TestPullSecretReconciler(t *testing.T) { | |||
| ctx := context.Background() | |||
|
|
|||
| clientFake := ctrlfake.NewClientBuilder().WithObjects(tt.instance).WithObjects(tt.secrets...).Build() | |||
| assert.NotNil(t, clientFake) | |||
There was a problem hiding this comment.
Do we need to add these extra assert calls here? The test works as-is without them added.
There was a problem hiding this comment.
It should be helpful to manifest nil object occurrence whenever that object is supposed to be not nil and thus to render test case failure early on.
| return reconcile.Result{}, err | ||
| } | ||
|
|
||
| err = r.client.Update(ctx, instance) | ||
| err = r.Client.Update(ctx, instance) | ||
| return reconcile.Result{}, err |
There was a problem hiding this comment.
We should validate if err is nil or not here, and:
- if err is nil, call the ClearConditions function to reset the controller back to Available: True, Progressing: False, Degraded: False
- if err is not nil, call SetDegraded to set the degraded status.
| @@ -285,11 +286,11 @@ func TestPullSecretReconciler(t *testing.T) { | |||
| ctx := context.Background() | |||
|
|
|||
| clientFake := ctrlfake.NewClientBuilder().WithObjects(tt.instance).WithObjects(tt.secrets...).Build() | |||
There was a problem hiding this comment.
We should also add validation in these tests to ensure that the conditions on the controller are what we expect (e.g. Degraded:True when an error occurs). You can see an example of this in the dnsmasq machine config controller tests: https://github.com/Azure/ARO-RP/blob/master/pkg/operator/controllers/dnsmasq/machineconfig_controller_test.go#L36
Which issue this PR addresses:
Fixes https://issues.redhat.com/browse/ARO-3258
What this PR does / why we need it:
It is needed to propagate errors of ARO PullSecret controller to ARO operator in order to see those errors at the Operator level and to see all the controller errors in one place.
Test plan for issue:
[x] Unit Test Cases
[x] Local Cluster Creation
[] CI
[] E2E
Are there unit tests
Yes, https://github.com/Azure/ARO-RP/blob/master/pkg/operator/controllers/pullsecret/pullsecret_controller_test.go
How do you know this will function as expected in production?
Running all unit tests locally and verifying tests output
Is there any documentation that needs to be updated for this PR?
N/A