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 1912888: Add recycler pod template as a ConfigMap #488
Bug 1912888: Add recycler pod template as a ConfigMap #488
Conversation
bertinatto
commented
Dec 14, 2020
•
edited
edited
- Reverts a45783f. This relied on a template file created by MCO, which updates after KCMO
- Creates a ConfigMap with the recycler template in it
- Projects the ConfigMap as a file to be used by the KCM pod
- Passes the projected template file to KCM as an extended arg
7dc9c5c
to
e7fc332
Compare
value: quay.io/openshift/origin-cluster-policy-controller:v4.3 | ||
value: quay.io/openshift/origin-cluster-policy-controller:latest | ||
- name: TOOLS_IMAGE | ||
value: quay.io/openshift/origin-tools:latest |
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.
This value is a placeholder for replacement mapped by https://github.com/openshift/cluster-kube-controller-manager-operator/blob/master/manifests/image-references. We need to add the tools image to that ImageStream
(and revert all the other changes to existing image values in this file, I assume were for testing)
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.
added
@@ -21,7 +21,3 @@ extendedArguments: | |||
service-cluster-ip-range: {{range .ServiceClusterIPRange}} | |||
- {{.}}{{end}} | |||
{{end}} | |||
pv-recycler-pod-template-filepath-nfs: # bootstrap KCM doesn't need recycler templates |
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.
why is this assumption no longer valid? If it isn't, you'll need to render out the results.
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.
I think it doesn't need it (the upstream template is used instead), but it's no harm if it has 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.
hm, the file isn't available during bootstrap, so it fails indeed. Removed
@@ -30,16 +30,16 @@ spec: | |||
--authentication-kubeconfig=/etc/kubernetes/static-pod-resources/configmaps/controller-manager-kubeconfig/kubeconfig \ | |||
--authorization-kubeconfig=/etc/kubernetes/static-pod-resources/configmaps/controller-manager-kubeconfig/kubeconfig \ | |||
--client-ca-file=/etc/kubernetes/static-pod-certs/configmaps/client-ca/ca-bundle.crt \ | |||
--requestheader-client-ca-file=/etc/kubernetes/static-pod-certs/configmaps/aggregator-client-ca/ca-bundle.crt | |||
--requestheader-client-ca-file=/etc/kubernetes/static-pod-certs/configmaps/aggregator-client-ca/ca-bundle.crt \ | |||
--pv-recycler-pod-template-filepath-hostpath=/etc/kubernetes/static-pod-resources/configmaps/kube-controller-manager-recycler-config/recycler-pod.yaml \ |
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.
@soltysh your call. I don't feel strongly about a move here versus the extended args. But half and half seems not-awsome.
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.
moved to extended args
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.
👍 for extended args
namespace: openshift-kube-controller-manager | ||
name: kube-controller-manager-recycler-config | ||
data: | ||
recycler-pod.yaml: |
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.
please directly embed your yaml here. Then you can simply create this configmap.
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.
done
b08fec5
to
91f5c6d
Compare
namespace: openshift-kube-controller-manager | ||
name: kube-controller-manager-recycler-config | ||
data: | ||
recycler-pod.yaml: |
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.
done
value: quay.io/openshift/origin-cluster-policy-controller:v4.3 | ||
value: quay.io/openshift/origin-cluster-policy-controller:latest | ||
- name: TOOLS_IMAGE | ||
value: quay.io/openshift/origin-tools:latest |
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.
added
@@ -21,7 +21,3 @@ extendedArguments: | |||
service-cluster-ip-range: {{range .ServiceClusterIPRange}} | |||
- {{.}}{{end}} | |||
{{end}} | |||
pv-recycler-pod-template-filepath-nfs: # bootstrap KCM doesn't need recycler templates |
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.
I think it doesn't need it (the upstream template is used instead), but it's no harm if it has it
- name: tools | ||
from: | ||
kind: DockerImage | ||
name: quay.io/openshift/origin-tools:latest |
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.
is there any harm in using latest
instead of pinning a version?
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.
the release image building process will replace this image pull path with the tools
image from that particular release
This reverts commit a45783f.
ce58a65
to
6ad7aa1
Compare
/test e2e-aws |
0c6d565
to
915a6e2
Compare
/hold cancel |
/retest |
@@ -12,9 +12,9 @@ extendedArguments: | |||
flex-volume-plugin-dir: | |||
- "/etc/kubernetes/kubelet-plugins/volume/exec" # created by machine-config-operator, owned by storage team/hekumar@redhat.com | |||
pv-recycler-pod-template-filepath-nfs: | |||
- "/etc/kubernetes/manifests/recycler-pod.yaml" # created by machine-config-operator, owned by storage team/fbertina@redhat.com | |||
- "/etc/kubernetes/static-pod-resources/configmaps/recycler-config/recycler-pod.yaml" |
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.
keep a storage team indication and name here.
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.
done
manifests/image-references
Outdated
@@ -14,3 +14,7 @@ spec: | |||
from: | |||
kind: DockerImage | |||
name: quay.io/openshift/origin-cluster-policy-controller:v4.3 | |||
- name: tools |
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.
keep a storage team indication and name here.
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.
done
@@ -404,6 +410,17 @@ func manageControllerManagerKubeconfig(ctx context.Context, client corev1client. | |||
return resourceapply.ApplyConfigMap(client, recorder, requiredCM) | |||
} | |||
|
|||
func manageRecycler(ctx context.Context, configMapsGetter corev1client.ConfigMapsGetter, recorder events.Recorder, imagePullSpec string) (*corev1.ConfigMap, bool, error) { |
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.
keep a storage team indication and name here.
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.
done
@bertinatto: This pull request references Bugzilla bug 1912888, 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. 3 validation(s) were run on this bug
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. |
@bertinatto: This pull request references Bugzilla bug 1896226, which is valid. 3 validation(s) were run on this bug
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. |
@bertinatto: No Bugzilla bug is referenced in the title of this pull request. 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. |
@bertinatto: This pull request references Bugzilla bug 1912888, which is invalid:
Comment 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. |
/bugzilla refresh |
@bertinatto: This pull request references Bugzilla bug 1912888, which is valid. 3 validation(s) were run on this bug
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. |
@bertinatto: All pull requests linked via external trackers have merged: Bugzilla bug 1912888 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. |
/cherry-pick 4.6 |
@bertinatto: cannot checkout 4.6: error checking out 4.6: exit status 1. output: error: pathspec '4.6' did not match any file(s) known to git 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. |
/cherry-pick release-4.6 |
@bertinatto: new pull request created: #490 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. |
The template is no longer necessary in MCO because now it's embedded in KCM operator [0]. [0] openshift/cluster-kube-controller-manager-operator#488