Skip to content

[OCPCLOUD-1162] Implement bootstrap aws#42

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
Danil-Grigorev:implement-bootstrap-aws
Jun 3, 2021
Merged

[OCPCLOUD-1162] Implement bootstrap aws#42
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
Danil-Grigorev:implement-bootstrap-aws

Conversation

@Danil-Grigorev
Copy link
Copy Markdown

@Danil-Grigorev Danil-Grigorev commented May 17, 2021

Following Pod will serve Node initialization during cluster bootstrap in AWS.

  • Running only --controllers=cloud-node as cloud-node controller contains node initialization parts we require in bootstrap.
apiVersion: v1
kind: Pod
metadata:
  name: aws-cloud-controller-manager
  namespace: kube-system
spec:
  priorityClassName: system-cluster-critical
  containers:
  - args:
    - --cloud-provider=aws
    - --use-service-account-credentials=false
    - --controllers=cloud-node # run only cloud-node controller required to bootstrap master nodes
    - --kubeconfig=/etc/kubernetes/secrets/kubeconfig
    - --leader-elect=false
    - -v=2
    image: gcr.io/k8s-staging-provider-aws/cloud-controller-manager:v1.19.0-alpha.1
    imagePullPolicy: IfNotPresent
    name: cloud-controller-manager
    volumeMounts:
    - mountPath: /etc/kubernetes/secrets
      name: secrets
      readOnly: true
  hostNetwork: true
  volumes:
  - hostPath:
      path: /etc/kubernetes/bootstrap-secrets
    name: secrets

@openshift-ci openshift-ci Bot requested review from Fedosin and mandre May 17, 2021 12:03
@Danil-Grigorev Danil-Grigorev force-pushed the implement-bootstrap-aws branch 3 times, most recently from eb86d6b to ca72dd6 Compare May 17, 2021 18:51
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2021
@Danil-Grigorev Danil-Grigorev force-pushed the implement-bootstrap-aws branch from ca72dd6 to 6cbfa7d Compare May 20, 2021 09:41
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2021
@Danil-Grigorev Danil-Grigorev force-pushed the implement-bootstrap-aws branch from 6cbfa7d to cd75cbd Compare May 20, 2021 10:05
@Danil-Grigorev
Copy link
Copy Markdown
Author

/retest

Comment thread pkg/cloud/aws/bootstrap/pod.yaml
Comment thread pkg/cloud/aws/bootstrap/pod.yaml
@lobziik
Copy link
Copy Markdown
Contributor

lobziik commented May 20, 2021

Overall approach make sense to me, but Pod resource is quite questionable ATM...

Comment thread pkg/cloud/aws/bootstrap/pod.yaml Outdated
- --kubeconfig=/etc/kubernetes/secrets/kubeconfig
- --leader-elect=false
- -v=2
- "2> /var/log/bootstrap-control-plane/cloud-controller-manager.log"
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the approach to store logs on bootstrap machine does not work. Neither this, nor --log-file option works in the CCM. I assume --log-file is an issue in cloud-provider. I would leave this line in, but we would need to address this issue to simplify CCM debug in bootstrap node. cc @JoelSpeed @lobziik

@Danil-Grigorev Danil-Grigorev requested a review from lobziik May 20, 2021 12:50
@Danil-Grigorev Danil-Grigorev force-pushed the implement-bootstrap-aws branch from cd75cbd to 8e0b6d3 Compare June 3, 2021 13:43
@lobziik
Copy link
Copy Markdown
Contributor

lobziik commented Jun 3, 2021

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2021
Copy link
Copy Markdown
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this generally makes sense to me, i just have a couple questions

Comment thread pkg/cloud/cloud_test.go
}, {
name: "Azure resources are empty, as the platform is not yet supported",
platform: configv1.AzurePlatformType,
}, {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have an entry for azure stack hub as well?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no platform for Azure Stack Hub :( We will have to implement additional logic to separate Azure and Azure Stack Hub similar to https://issues.redhat.com/browse/OCPCLOUD-1187 (well, starting from that, and then read the full infra status in operator as well to get it right).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, thanks!

Comment thread pkg/cloud/common/sources.go Outdated
Comment thread pkg/cloud/common/sources.go
- Document methods for getting resources for the <provider> platform
@Danil-Grigorev Danil-Grigorev force-pushed the implement-bootstrap-aws branch from 8e0b6d3 to 5dee6c6 Compare June 3, 2021 15:22
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2021
@lobziik
Copy link
Copy Markdown
Contributor

lobziik commented Jun 3, 2021

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2021
Copy link
Copy Markdown
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 3, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 3, 2021

@Danil-Grigorev: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-ccm 5dee6c6 link /test e2e-aws-ccm

Full PR test history. Your PR dashboard.

Details

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.

@openshift-bot
Copy link
Copy Markdown

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 81ed17e into openshift:master Jun 3, 2021
@Danil-Grigorev Danil-Grigorev changed the title Implement bootstrap aws [OCPCLOUD-1162] Implement bootstrap aws Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants