Skip to content

Conversation

stephenfin
Copy link
Contributor

@stephenfin stephenfin commented Apr 15, 2025

The generator has the ability to generate kube-rbac-proxy sidecar containers when exposing driver metrics on the controller or node.

In #379, we noted that while the sidecar containers templates used template variables, the actual driver template used hardcoded strings. This allows for the possibility of bugs if the port numbers are changed at any point.

Resolve this by modifying the generator such that the metric port for the drivers is now templated also. Changes are implemented in multiple steps: I'd encourage reviewers to look at the individual commits.

This includes the commits from #379 and that can as such be considered a dependency. That PR is now merged.

Dependencies:

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 15, 2025
@openshift-ci-robot
Copy link

@stephenfin: This pull request explicitly references no jira issue.

In response to this:

The generator has the ability to generate kube-rbac-proxy sidecar containers when exposing driver metrics on the controller or node.
In #379, we noted that while the sidecar containers templates used template variables, the actual driver template used hardcoded strings. This allows for the possibility of bugs if the port numbers are changed at any point.
Resolve this by modifying the generator such that the metric port for the drivers is now templated also. Changes are implemented in multiple steps: I'd encourage reviewers to look at the individual commits.

This includes the commits from #379 and that can as such be considered a dependency.

Dependencies:

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot requested review from dobsonj and mandre April 15, 2025 14:07
@stephenfin stephenfin force-pushed the cleanup-driver-metrics branch from 9ebb32d to 4c9db2b Compare May 14, 2025 11:08
@stephenfin
Copy link
Contributor Author

/retest

@stephenfin stephenfin force-pushed the cleanup-driver-metrics branch from 4c9db2b to 917c3be Compare May 30, 2025 10:48
@stephenfin
Copy link
Contributor Author

/uncc @dobsonj
/cc @gnufied
/cc @mandre

@openshift-ci openshift-ci bot requested review from gnufied and mandre and removed request for dobsonj May 30, 2025 10:49
@stephenfin stephenfin force-pushed the cleanup-driver-metrics branch from 917c3be to 2bfa3a1 Compare June 24, 2025 13:12
Copy link
Contributor

openshift-ci bot commented Jun 24, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stephenfin

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 Jun 24, 2025
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 23, 2025
No need to use a slice.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin stephenfin force-pushed the cleanup-driver-metrics branch from 2bfa3a1 to 410d043 Compare September 24, 2025 11:48
The 'InjectKubeRBACProxy' field was was always set to True in users and
was therefore a no-op. The 'Name' field was likewise always set to
'driver-m' and doesn't need to be templated. Flatten the remaining two
fields into the parent structs and remove MetricsPorts entirely.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin stephenfin force-pushed the cleanup-driver-metrics branch from 410d043 to 6764a2c Compare September 24, 2025 11:50
@stephenfin
Copy link
Contributor Author

/remove-lifecycle stale

@openshift-ci openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 24, 2025
@stephenfin
Copy link
Contributor Author

/retest-required

There's no obvious reason for those clusters not to have come up

@stephenfin stephenfin force-pushed the cleanup-driver-metrics branch from 6764a2c to a4dcd7d Compare September 24, 2025 15:37
@stephenfin
Copy link
Contributor Author

I've dropped the last commit since the rest of these changes are no-ops, as seen in the lack of changes to the assets. I'll propose that last commit separately.

@stephenfin
Copy link
Contributor Author

Well now we know it's unhappy CI(s): there are zero changes to the generated assets in this PR now.

/retest-required

@stephenfin
Copy link
Contributor Author

/test aws-efs-operator-e2e

@stephenfin
Copy link
Contributor Author

/test aws-efs-operator-e2e

@stephenfin
Copy link
Contributor Author

/test e2e-openstack

fwiw, these are happening because the cloud we are using has multiple CI systems (not just Prow) using it, and the other CI systems are stealing our resources. We are working overtime to get this resolved, but it involves multiple teams 😞

@jsafrane
Copy link
Contributor

/lgtm
/verified by CI

There is no difference in the output of the generator, hence there is not much to test.

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Sep 30, 2025
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2025
@openshift-ci-robot
Copy link

@jsafrane: This PR has been marked as verified by CI.

In response to this:

/lgtm
/verified by CI

There is no difference in the output of the generator, hence there is not much to test.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD b52e702 and 2 for PR HEAD a4dcd7d in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 12ef6e8 and 1 for PR HEAD a4dcd7d in total

@stephenfin
Copy link
Contributor Author

/override "hypershift-e2e-openstack-aws-csi-cinder"

We know this job is failing. We are working on the fix with openshift/hypershift#6909 and openshift/release#69631 but until then, as mentioned multiple times this doesn't affect generated assets and is a no-op from the production perspective.

Copy link
Contributor

openshift-ci bot commented Oct 1, 2025

@stephenfin: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • hypershift-e2e-openstack-aws-csi-cinder

Only the following failed contexts/checkruns were expected:

  • ci/prow/aws-efs-operator-e2e
  • ci/prow/ci-index-aws-efs-csi-driver-operator-bundle
  • ci/prow/ci-index-smb-csi-driver-operator-bundle
  • ci/prow/e2e-aws-csi
  • ci/prow/e2e-aws-csi-extended
  • ci/prow/e2e-aws-ovn-upgrade
  • ci/prow/e2e-azure
  • ci/prow/e2e-azure-csi
  • ci/prow/e2e-azure-csi-extended
  • ci/prow/e2e-azure-file-csi
  • ci/prow/e2e-azure-file-csi-extended
  • ci/prow/e2e-azure-file-nfs-csi
  • ci/prow/e2e-azure-ovn-upgrade
  • ci/prow/e2e-azurestack-csi
  • ci/prow/e2e-openstack
  • ci/prow/e2e-openstack-cinder-csi
  • ci/prow/hypershift-aws-e2e-external
  • ci/prow/hypershift-e2e-aks
  • ci/prow/hypershift-e2e-openstack-aws-csi-cinder
  • ci/prow/images
  • ci/prow/okd-scos-e2e-aws-ovn
  • ci/prow/okd-scos-images
  • ci/prow/security
  • ci/prow/smb-operator-e2e
  • ci/prow/smb-operator-e2e-extended
  • ci/prow/smb-win2019-operator-e2e
  • ci/prow/smb-win2022-operator-e2e
  • ci/prow/unit
  • ci/prow/verify
  • ci/prow/verify-deps
  • pull-ci-openshift-csi-operator-main-aws-efs-operator-e2e
  • pull-ci-openshift-csi-operator-main-ci-index-aws-efs-csi-driver-operator-bundle
  • pull-ci-openshift-csi-operator-main-ci-index-smb-csi-driver-operator-bundle
  • pull-ci-openshift-csi-operator-main-e2e-aws-csi
  • pull-ci-openshift-csi-operator-main-e2e-aws-csi-extended
  • pull-ci-openshift-csi-operator-main-e2e-aws-ovn-upgrade
  • pull-ci-openshift-csi-operator-main-e2e-azure
  • pull-ci-openshift-csi-operator-main-e2e-azure-csi
  • pull-ci-openshift-csi-operator-main-e2e-azure-csi-extended
  • pull-ci-openshift-csi-operator-main-e2e-azure-file-csi
  • pull-ci-openshift-csi-operator-main-e2e-azure-file-csi-extended
  • pull-ci-openshift-csi-operator-main-e2e-azure-file-nfs-csi
  • pull-ci-openshift-csi-operator-main-e2e-azure-ovn-upgrade
  • pull-ci-openshift-csi-operator-main-e2e-azurestack-csi
  • pull-ci-openshift-csi-operator-main-e2e-openstack
  • pull-ci-openshift-csi-operator-main-e2e-openstack-cinder-csi
  • pull-ci-openshift-csi-operator-main-hypershift-aws-e2e-external
  • pull-ci-openshift-csi-operator-main-hypershift-e2e-aks
  • pull-ci-openshift-csi-operator-main-hypershift-e2e-openstack-aws-csi-cinder
  • pull-ci-openshift-csi-operator-main-images
  • pull-ci-openshift-csi-operator-main-okd-scos-e2e-aws-ovn
  • pull-ci-openshift-csi-operator-main-okd-scos-images
  • pull-ci-openshift-csi-operator-main-security
  • pull-ci-openshift-csi-operator-main-smb-operator-e2e
  • pull-ci-openshift-csi-operator-main-smb-operator-e2e-extended
  • pull-ci-openshift-csi-operator-main-smb-win2019-operator-e2e
  • pull-ci-openshift-csi-operator-main-smb-win2022-operator-e2e
  • pull-ci-openshift-csi-operator-main-unit
  • pull-ci-openshift-csi-operator-main-verify
  • pull-ci-openshift-csi-operator-main-verify-deps
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override "hypershift-e2e-openstack-aws-csi-cinder"

We know this job is failing. We are working on the fix with openshift/hypershift#6909 and openshift/release#69631 but until then, as mentioned multiple times this doesn't affect generated assets and is a no-op from the production perspective.

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-sigs/prow repository.

@stephenfin
Copy link
Contributor Author

/override "ci/prow/hypershift-e2e-openstack-aws-csi-cinder"

Copy link
Contributor

openshift-ci bot commented Oct 1, 2025

@stephenfin: Overrode contexts on behalf of stephenfin: ci/prow/hypershift-e2e-openstack-aws-csi-cinder

In response to this:

/override "ci/prow/hypershift-e2e-openstack-aws-csi-cinder"

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-sigs/prow repository.

@stephenfin
Copy link
Contributor Author

/retest-required

Oof

@stephenfin
Copy link
Contributor Author

/retest-required

1 similar comment
@stephenfin
Copy link
Contributor Author

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD f99af07 and 0 for PR HEAD a4dcd7d in total

@openshift-ci-robot
Copy link

/hold

Revision a4dcd7d was retested 3 times: holding

@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 8, 2025
Copy link
Contributor

openshift-ci bot commented Oct 8, 2025

@stephenfin: 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/e2e-azurestack-csi a4dcd7d link false /test e2e-azurestack-csi
ci/prow/okd-scos-e2e-aws-ovn a4dcd7d link false /test okd-scos-e2e-aws-ovn
ci/prow/e2e-azure-file-csi-extended a4dcd7d link false /test e2e-azure-file-csi-extended
ci/prow/e2e-openstack a4dcd7d link false /test e2e-openstack
ci/prow/e2e-azure-csi a4dcd7d link true /test e2e-azure-csi
ci/prow/e2e-azure-file-nfs-csi a4dcd7d link true /test e2e-azure-file-nfs-csi
ci/prow/e2e-azure-file-csi a4dcd7d link true /test e2e-azure-file-csi
ci/prow/e2e-openstack-cinder-csi a4dcd7d link true /test e2e-openstack-cinder-csi
ci/prow/aws-efs-operator-e2e a4dcd7d link true /test aws-efs-operator-e2e

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-sigs/prow repository. I understand the commands that are listed here.

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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants