Move image registry operator to control plane #1643
Conversation
openshift-ci
bot
added
the
approved
| } | ||
|
|
||
| switch params.platform { | ||
| case hyperv1.AWSPlatform: |
There was a problem hiding this comment.
so on azure this isn't going to work initially since there are no creds? If so, lets maybe create a small reminder jira issue to fix that up later?
There was a problem hiding this comment.
The creds for azure are already on the guest cluster since the hcco puts them there. The operator should read them as usual. We have a special case here for AWS because we need a token minter instead of using the token request volume in the podspec
| sleep 2 | ||
| done | ||
|
|
||
| echo "{{ .WorkerNamespace }}" > "{{ .TokenDir }}/namespace" |
There was a problem hiding this comment.
Nit: We can get the namespace through the downwards api and that in turn would make the template variables all static and allow us to render it in an init or var declaration
There was a problem hiding this comment.
Actually all vars are static... I will change to generating this in an init func.
| } | ||
|
|
||
| /* | ||
| Command: []string{"/usr/bin/control-plane-operator", "token-minter"}, |
| svc.Spec.Selector = selectorLabels() | ||
| } | ||
|
|
||
| func ReconcileServiceMonitor(sm *prometheusoperatorv1.ServiceMonitor, clusterID string, metricsSet metrics.MetricsSet) { |
There was a problem hiding this comment.
If we use a podmonitor, we do not need the service
| cp "/etc/certificate/ca/ca.crt" "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" | ||
| export KUBERNETES_SERVICE_HOST=kube-apiserver | ||
| export KUBERNETES_SERVICE_PORT=$KUBE_APISERVER_SERVICE_PORT | ||
| /usr/bin/cluster-image-registry-operator \ |
There was a problem hiding this comment.
Nit to stop running the shell:
| /usr/bin/cluster-image-registry-operator \ | |
| exec /usr/bin/cluster-image-registry-operator \ |
| targetPort: metrics | ||
| tlsConfig: | ||
| ca: | ||
| secret: |
There was a problem hiding this comment.
Just for my understanding, does this mean that whatever does the scraping needs permissions to read secrets? I think it would be good if we could avoid that
There was a problem hiding this comment.
we already require that in other service monitors (kas, kcm, etc) because we authenticate with the KAS via mtls to access the metrics endpoint.
csrwng
force-pushed
the
move-registry-operator
branch
2 times, most recently
from
78f0e91 to
90b3f3a
Compare
|
@alvaroaleman thx for the review. Addressed your comments. |
|
/retest-required |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
taking a look |
Stops CVO from reconciling the cluster image registry operator into the guest cluster and enables the control plane operator to reconcile it on the management cluster side. This change is needed to make it possible to clean up resources created by the guest cluster. Having the image registry operator on the control plane side allows it to keep running after worker nodes have been removed from the cluster. On the guest cluster, changing the operator config state to "Removed" tells the operator to remove the S3 bucket (or other cloud storage) it created when starting up.
csrwng
force-pushed
the
move-registry-operator
branch
from
90b3f3a to
d529f6d
Compare
|
Fixed the upgrade path by adding the operator deployment to the set of manifests to delete and modifying the name of the cleanup manifest so that the cvo applies it first. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, csrwng 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 |
|
/hold cc @hasueki (I think this is likely only going into 4.12: but still something that should be accounted for). |
|
/retest-required |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@csrwng: all tests passed! 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. |
What this PR does / why we need it:
Stops CVO from reconciling the cluster image registry operator into the
guest cluster and enables the control plane operator to reconcile it on
the management cluster side.
This change is needed to make it possible to clean up resources created
by the guest cluster. Having the image registry operator on the control
plane side allows it to keep running after worker nodes have been
removed from the cluster. On the guest cluster, changing the operator
config state to "Removed" tells the operator to remove the S3 bucket
(or other cloud storage) it created when starting up.
Which issue(s) this PR fixes (optional, use
fixes #<issue_number>(, fixes #<issue_number>, ...)format, where issue_number might be a GitHub issue, or a Jira story:Fixes #HOSTEDCP-486
Checklist