[EV-5299] Disable Policy Recommendation for Windows OS #3563
Conversation
| @@ -430,6 +431,12 @@ func (r *ReconcilePolicyRecommendation) Reconcile(ctx context.Context, request r | |||
| return reconcile.Result{}, nil | |||
| } | |||
|
|
|||
| // Render a Policy Recommendation component for Windows if the cluster has Windows nodes. | |||
There was a problem hiding this comment.
If I understand it correctly, we cannot have a cluster using policy recommendation and windows nodes at the same time, because windows does not support staged policies.
If that is the case, we should degrade the TigeraStatus immediately if hasWindowsNodes is true. I think we can move this if-statement further up, so we may save some unnecessary calculations, perhaps right underneath the ready checks at line 291.
There was a problem hiding this comment.
Moved the check to below line 291.
It isn't used, but I've kept the check in the render to enforce a defensive pattern
dimitri-nicolo
force-pushed
the
dimitri-EV-5299
branch
from
10ff273
to
5231a47
Compare
| return reconcile.Result{}, err | ||
| } | ||
| if hasWindowsNodes { | ||
| r.status.SetDegraded(operatorv1.ResourceValidationError, "Unsupported OS (Windows)", err, logc) |
There was a problem hiding this comment.
I was wondering if we could make it a bit more descriptive, so they understand well what's going on.
Policy Recommendation is not supported in combination with Windows nodes.
| // Render a Policy Recommendation component for Windows if the cluster has Windows nodes. | ||
| hasWindowsNodes, err := common.HasWindowsNodes(r.client) | ||
| if err != nil { | ||
| r.status.SetDegraded(operatorv1.ResourceNotReady, "OS not found", err, logc) |
There was a problem hiding this comment.
Judging by the implementation of HashWindowsNodes, I think it should be a ResourceReadError and perhaps the following message would be better.
Unable to determine if there are Windows nodes.
|
/merge-when-ready delete-branch |
|
OK, I will merge the pull request when it's ready, leave the commits as is when I merge it, and delete the branch after I've merged it. |
dimitri-nicolo
force-pushed
the
dimitri-EV-5299
branch
from
54494ff
to
6534f69
Compare
|
Removing "merge-when-ready" label due to new commits |
dimitri-nicolo
force-pushed
the
dimitri-EV-5299
branch
from
6534f69
to
f0e4812
Compare
|
/merge-when-ready |
|
OK, I will merge the pull request when it's ready, leave the commits as is when I merge it, and leave the branch after I've merged it. |
dimitri-nicolo
force-pushed
the
dimitri-EV-5299
branch
from
f0e4812
to
30ad410
Compare
|
Removing "merge-when-ready" label due to new commits |
Description
Disable Image Resolution for Windows
For PR author
make gen-filesmake gen-versionsFor PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bugif this is a bugfix.kind/enhancementif this is a a new feature.enterpriseif this PR applies to Calico Enterprise only.