Azure CCM assets#62
Azure CCM assets#62openshift-merge-robot merged 2 commits intoopenshift:masterfrom lobziik:azure-assets
Conversation
|
cc @Fedosin @elmiko @JoelSpeed Folks, PTAL when you will have time :) |
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
/approve
Looks ok to me, how has this been tested?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/hold Found problem with cloud-conf syncing |
openshift-ci
Bot
added
the
do-not-merge/hold
| priorityClassName: system-cluster-critical | ||
| nodeSelector: | ||
| node-role.kubernetes.io/master: "" | ||
| tolerations: |
There was a problem hiding this comment.
does this also need to tolerate not-ready ?
seems like aws does, https://github.com/openshift/cluster-cloud-controller-manager-operator/blob/master/pkg/cloud/aws/assets/deployment.yaml#L55
There was a problem hiding this comment.
None of our components should be tolerating that taint, if we do, we will schedule onto a node without networking, which will likely cause our pods to crash loop as they can't contact the APIs they need to contact
There was a problem hiding this comment.
It will now need to tolerate the not-ready based on the additions made in #76
| hostNetwork: true # required to fetch correct hostname | ||
| nodeSelector: | ||
| kubernetes.io/os: linux | ||
| tolerations: |
There was a problem hiding this comment.
same question here, i'm guessing since this uses the host network that it will need to tolerate not-ready as well.
There was a problem hiding this comment.
Host network maybe means we can allow it to tolerate the taint, though IMO, we should only add the toleration if we are certain we need it. I'd suggest being conservative initially
|
Updated, depends on #86 now. Changes with inline arguments and check for CNI was also ported there |
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
You'll need to look at the test failures here and make sure you are meeting the spec for the daemonset and deployment pod specs https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-cloud-controller-manager-operator/62/pull-ci-openshift-cluster-cloud-controller-manager-operator-master-verify/1413158319933100032#1:build-log.txt%3A278
| priorityClassName: system-cluster-critical | ||
| nodeSelector: | ||
| node-role.kubernetes.io/master: "" | ||
| tolerations: |
There was a problem hiding this comment.
It will now need to tolerate the not-ready based on the additions made in #76
|
#77 |
| labels: | ||
| app: azure-cloud-controller-manager | ||
| spec: | ||
| hostNetwork: true |
There was a problem hiding this comment.
Suppose hostNetwork is not required for other controllers except cloud-node
openshift-ci
Bot
added
needs-rebase
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/lgtm |
|
@lobziik: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
6 participants
depends on #86