Skip to content
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

STOR-1037: Add cluster-storage-operator to mgmt. cluster #1748

Merged
merged 6 commits into from Dec 9, 2022

Conversation

jsafrane
Copy link
Contributor

@jsafrane jsafrane commented Sep 16, 2022

What this PR does / why we need it:

Move cluster-storage-operator + its operands (aws-ebs-csi-driver-operator + AWS EBS CSI driver control plane pods) to HyperShift mgmt. cluster.

cluster-storage-operator must be updated first!

Testing
Update control-plane-operator RBACs:

  1. Build your own hypershift-operator image.
    1. Clone openshift/hypershift git repo + apply this PR.
    2. Build the custom image, e.g. quay.io/jsafrane/hypershift:latest:
      make docker-build docker-push quay.io/jsafrane/hypershift:latest
  2. Install hypershift-operator from the custom image: ./hypershift install --hypershift-image=quay.io/jsafrane/hypershift:latest ...

Install a guest cluster:

  1. Build a release that contains this PR + cluster-storage-operator PR + AWS operator PR. For example, ask cluster-bot: build openshift/hypershift#1698,openshift/aws-ebs-csi-driver-operator#159,openshift/cluster-storage-operator#XXX
  2. Use the release to install a guest cluster: ./hypershift create cluster aws --release-image=registry.build02.ci.openshift.org/ci-ln-ww5ihgt/release:latest ...

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2022
@netlify
Copy link

netlify bot commented Sep 29, 2022

Deploy Preview for hypershift-docs ready!

Name Link
🔨 Latest commit a020c10
🔍 Latest deploy log https://app.netlify.com/sites/hypershift-docs/deploys/63526dc4467fee0009800da0
😎 Deploy Preview https://deploy-preview-1748--hypershift-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

- name: POWERVS_BLOCK_CSI_DRIVER_IMAGE
value: quay.io/openshift/origin-powervs-block-csi-driver:latest
- name: HYPERSHIFT_IMAGE
value: tbd
Copy link
Member

Choose a reason for hiding this comment

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

I thought we had this - quay.io/openshift/origin-control-plane:latest ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@jsafrane jsafrane changed the title WIP: STOR-1037: Add cluster-storage-operator to mgmt. cluster STOR-1037: Add cluster-storage-operator to mgmt. cluster Oct 10, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 10, 2022
@jsafrane
Copy link
Contributor Author

jsafrane commented Oct 10, 2022

/hold
openshift/cluster-storage-operator#318 must be merged first!

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 10, 2022
@@ -802,7 +802,7 @@ func (o HyperShiftOperatorClusterRole) Build() *rbacv1.ClusterRole {
},
{
APIGroups: []string{"apps"},
Resources: []string{"deployments", "statefulsets"},
Resources: []string{"deployments", "replicasets", "statefulsets"},
Copy link
Member

Choose a reason for hiding this comment

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

Note for myself: this will likely require refresh the HO CI role.


func ClusterStorageOperatorRole(ns string) *rbacv1.Role {
role := &rbacv1.Role{}
role.Name = "cluster-storage-operator-role"
Copy link
Member

Choose a reason for hiding this comment

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

name is redundant with type "cluster-storage-operator-role" -> "cluster-storage-operator"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed


func ClusterStorageOperatorRoleBinding(ns string) *rbacv1.RoleBinding {
roleBinding := &rbacv1.RoleBinding{}
roleBinding.Name = "cluster-storage-operator-role"
Copy link
Member

Choose a reason for hiding this comment

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

cluster-storage-operator-role-> cluster-storage-operator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed

@@ -0,0 +1,137 @@
# *** AUTOMATICALLY GENERATED FILE - DO NOT EDIT ***
Copy link
Member

Choose a reason for hiding this comment

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

can you ref the source? automatically generated by what/how?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a README.md

kind: RoleBinding
metadata:
labels:
name: cluster-storage-operator-role
Copy link
Member

Choose a reason for hiding this comment

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

cluster-storage-operator-role-> cluster-storage-operator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed

)

var (
// map env. variable in CSO Deployment -> key in `images` map (= name of the image in payload).
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any programatic guarantee that we don't end up with the operator referencing an image as hardcoded in the yaml, e.g. if it's missing here?
Can we unit test it somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reworked the env. var replacement into its own file + added an unit test

@enxebre
Copy link
Member

enxebre commented Oct 20, 2022

/label tide/merge-method-squash
I dropped some minor comments.
Looks great to me otherwise, pending openshift/cluster-storage-operator#318

@openshift-ci openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 20, 2022
@sferich888
Copy link

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Oct 20, 2022
@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Oct 29, 2022
@jsafrane
Copy link
Contributor Author

jsafrane commented Nov 1, 2022

/retest
do we have 4.11.z fixed?

@jsafrane
Copy link
Contributor Author

jsafrane commented Nov 2, 2022

/test e2e-aws

@jsafrane
Copy link
Contributor Author

/retest

1 similar comment
@tsmetana
Copy link
Member

/retest

@csrwng
Copy link
Contributor

csrwng commented Nov 14, 2022

/retest-required

@enxebre
Copy link
Member

enxebre commented Nov 16, 2022

/test e2e-aws

@enxebre
Copy link
Member

enxebre commented Nov 16, 2022

@jsafrane failure is legit.

=== RUN TestUpgradeControlPlane/EnsureNoPodsWithTooHighPriority
util.go:411: pod aws-ebs-csi-driver-controller-7fb697944c-8pnzm with priorityClassName system-cluster-critical has a priority of 2000000000 with exceeds the max allowed of 100002000

It will be installed by control-plane-operator in the mgmt cluster.
Right now, the operator needs more permissions that would be necessary, but
we need to fix cluster-storage-operator and aws-ebs-csi-driver-operators
first.
@jsafrane
Copy link
Contributor Author

jsafrane commented Dec 8, 2022

From guest clusters:

message: Cluster operators console, dns, image-registry, ingress, insights,
kube-storage-version-migrator, monitoring, node-tuning, openshift-samples,
service-ca, storage are not available

I don't see any nodes created for the guest clusters.
/retest

@csrwng
Copy link
Contributor

csrwng commented Dec 8, 2022

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 8, 2022

@jsafrane: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/periodics-4.13-images a020c10 link true /test periodics-4.13-images
ci/prow/e2e-ibmcloud-iks a020c10 link false /test e2e-ibmcloud-iks
ci/prow/e2e-ibmcloud-roks a020c10 link false /test e2e-ibmcloud-roks
ci/prow/kubevirt-e2e-kubevirt-azure-ovn 99a5e90 link false /test kubevirt-e2e-kubevirt-azure-ovn
ci/prow/capi-provider-agent-sanity 99a5e90 link false /test capi-provider-agent-sanity

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.

@csrwng
Copy link
Contributor

csrwng commented Dec 9, 2022

/approve

@csrwng
Copy link
Contributor

csrwng commented Dec 9, 2022

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 9, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, jsafrane

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 /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 Dec 9, 2022
@csrwng
Copy link
Contributor

csrwng commented Dec 9, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2022
@openshift-merge-robot openshift-merge-robot merged commit 6d862d1 into openshift:main Dec 9, 2022
@derekwaynecarr
Copy link
Member

woot!

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. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants