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

Add Attached devices flag to ceph plugin #6069

Merged

Conversation

afreen23
Copy link
Contributor

@afreen23 afreen23 commented Jul 22, 2020

  • The flag name is OCS_ATTACHED_DEVICES
  • The flag will allow to detect ocs installation on baremetal
  • The flag will be consumed for disk replacement kebab action in 4.6
  • Updated Readme
  • https://issues.redhat.com/browse/RHSTOR-1196

@openshift-ci-robot openshift-ci-robot added the component/ceph Related to ceph-storage-plugin label Jul 22, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2020
@afreen23
Copy link
Contributor Author

/hold need engg discussions

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2020
@afreen23 afreen23 force-pushed the add-attached-devices-flag-ocs branch 3 times, most recently from 130d3f5 to 62f9193 Compare July 23, 2020 04:00
@afreen23
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2020
Copy link
Contributor

@bipuladh bipuladh left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2020
@openshift-bot
Copy link
Contributor

/retest

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

Comment on lines 50 to 57
requestData.spec.monDataDirHostPath = '/var/lib/rook';
requestData.spec.storageDeviceSets[0].portable = false;
requestData.metadata = {
...requestData.metadata,
annotations: {
[PLATFORM_ANNOTATION]: 'true',
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We should these as args of the request data generator function. No point in having a function if we have to modify again and again.

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 could not do refactors for this PR. Just using an existing functionality.

Copy link
Contributor Author

@afreen23 afreen23 Jul 23, 2020

Choose a reason for hiding this comment

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

Not relevant to this PR, but I think we should make the request function generic.
Post the install PR or in the install PR itself it can be considered. @gnehapk ^^

@@ -15,3 +15,4 @@ export const OCS_INTERNAL_CR_NAME = 'ocs-storagecluster';
export const NO_PROVISIONER = 'kubernetes.io/no-provisioner';
export const OCS_SUPPORT_ANNOTATION = 'features.ocs.openshift.io/enabled';
export const OCS_DEVICE_SET_REPLICA = 3;
export const PLATFORM_ANNOTATION = 'cluster.ocs.openshift.io/local-devices';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const PLATFORM_ANNOTATION = 'cluster.ocs.openshift.io/local-devices';
export const ATTACH_DEVICE_ANNOTATION = 'cluster.ocs.openshift.io/local-devices';

@afreen23
Copy link
Contributor Author

afreen23 commented Jul 23, 2020

Need to update README
/lgtm cancel

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2020
@afreen23 afreen23 force-pushed the add-attached-devices-flag-ocs branch from 62f9193 to 7dc8906 Compare July 23, 2020 06:09
- The flag name is OCS_ATTACHED_DEVICES
- Its enabled on the annotation: cluster.ocs.openshift.io/local-devices
- The flag will allow to detect ocs installation on baremetal
- The flag will be consumed for disk replacement kebab action in 4.6
- Updates readme
- https://issues.redhat.com/browse/RHSTOR-1196

Signed-off-by: Afreen Rahman <afrahman@redhat.com>
@afreen23 afreen23 force-pushed the add-attached-devices-flag-ocs branch from 7dc8906 to a415b82 Compare July 23, 2020 06:48
@cloudbehl
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, cloudbehl

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-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 6906cfa into openshift:master Jul 23, 2020
@spadgett spadgett added this to the v4.6 milestone Jul 28, 2020
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. component/ceph Related to ceph-storage-plugin lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants