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

allow csi-attacher component to be configured #516

Closed

Conversation

nschad
Copy link
Contributor

@nschad nschad commented Nov 23, 2022

How to categorize this PR?

/kind enhancement
/kind api-change
/area storage
/platform openstack

What this PR does / why we need it:

Enables the configuration of the csi-attacher component by providing 5 additional flags for providing timeout and retries in case the storage is not so fast.

Example config of the gardener-extension-provider-openstack.

csi:
  csiAttacher:
    timeout: 3m
    verbosity: 5
    retryIntervalStart: 1m
    reconcileSync: 15m
    retryIntervalMax: 5m

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Enables part-configuration of the csi-attacher (--retry-interval-max, --retry-interval-start, --reconcile-sync, --timeout, --v) component for all shoots.

@gardener-robot gardener-robot added kind/api-change API change with impact on API users needs/second-opinion Needs second review by someone else area/storage Storage related kind/enhancement Enhancement, improvement, extension platform/openstack OpenStack platform/infrastructure labels Nov 23, 2022
@gardener-robot
Copy link

@nschad Thank you for your contribution.

@gardener-robot gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) labels Nov 23, 2022
@gardener-robot-ci-2
Copy link
Contributor

Thank you @nschad for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@nschad nschad force-pushed the feature/cinder-csi-parameters branch from 199d5d5 to 716ccfd Compare November 24, 2022 15:41
@nschad nschad marked this pull request as ready for review November 24, 2022 15:41
@nschad nschad requested review from a team as code owners November 24, 2022 15:41
@timebertt timebertt added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 25, 2022
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 25, 2022
@nschad
Copy link
Contributor Author

nschad commented Nov 25, 2022

Ah yeah my make generate generate a different output (no idea why really, it does work though). I will see if if I can fix that somehow

Should be resolved hopefully

@nschad nschad force-pushed the feature/cinder-csi-parameters branch 2 times, most recently from 5571f5d to a9477d6 Compare November 27, 2022 22:44
@timebertt timebertt added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 29, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 29, 2022
Copy link
Member

@timebertt timebertt left a comment

Choose a reason for hiding this comment

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

Some nits from reviewing the first two commits.
Can you check the CI failures and run make verify locally?

pkg/apis/config/types.go Show resolved Hide resolved
pkg/apis/config/v1alpha1/defaults.go Show resolved Hide resolved
@nschad nschad force-pushed the feature/cinder-csi-parameters branch from a9477d6 to 8695921 Compare December 1, 2022 08:12
@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Dec 1, 2022
@gardener-robot
Copy link

@nschad You need rebase this pull request with latest master branch. Please check.

@nschad nschad force-pushed the feature/cinder-csi-parameters branch from 8695921 to a6650b8 Compare December 14, 2022 16:44
Niclas Schad added 2 commits December 14, 2022 17:53
Signed-off-by: Niclas Schad <niclas.schad@stackit.de>
Signed-off-by: Niclas Schad <niclas.schad@stackit.de>
Niclas Schad added 3 commits December 14, 2022 17:53
Signed-off-by: Niclas Schad <niclas.schad@stackit.de>
Signed-off-by: Niclas Schad <niclas.schad@stackit.de>
Signed-off-by: Niclas Schad <niclas.schad@stackit.de>
@nschad nschad force-pushed the feature/cinder-csi-parameters branch from a6650b8 to ba6c787 Compare December 14, 2022 17:07
@kon-angelo
Copy link
Contributor

The PR itself looks okay, however given #572 maybe we would need a more "generic" way to map an attacher configuration to a CSI driver and not have this as a "global" setting for all drivers. WDYT?

@nschad
Copy link
Contributor Author

nschad commented Feb 15, 2023

The PR itself looks okay, however given #572 maybe we would need a more "generic" way to map an attacher configuration to a CSI driver and not have this as a "global" setting for all drivers. WDYT?

I agree. I made some suggestions in my last PR. However they were all refused. I don't see how to create a generic config while checking for valid flags.

Personally I don't like how this PR is implemented.

@gardener-robot gardener-robot added the lifecycle/stale Nobody worked on this for 6 months (will further age) label Oct 26, 2023
@gardener-robot gardener-robot added lifecycle/rotten Nobody worked on this for 12 months (final aging stage) and removed lifecycle/stale Nobody worked on this for 6 months (will further age) labels Jul 4, 2024
@nschad nschad closed this Aug 15, 2024
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/storage Storage related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension lifecycle/rotten Nobody worked on this for 12 months (final aging stage) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase needs/review Needs review needs/second-opinion Needs second review by someone else platform/openstack OpenStack platform/infrastructure size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants