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

deploy: remove external-attacher sidecar from cephfs deployment #3149

Merged
merged 3 commits into from
Jun 6, 2022

Conversation

humblec
Copy link
Collaborator

@humblec humblec commented Jun 1, 2022

CephFS CSI driver dont have/advertise controller publish/unpublish capabilities, thus dont need attacher sidecar for its operations.
Also this removal take care of the burden on unncessary operation (round-trip) in the preparation of a volume for a container, and requires CSI Drivers to deploy an unnecessary sidecar container (external-attacher).

Things covered in this PR

  • Removed attacher sidecar from deploy yamls and also from Helm
  • Removed/adjusted RBAC supporting this removal
  • CephFS CSI driver object have been changed to attachRequired:false param as attachment is no longer required.

Signed-off-by: Humble Chirammal [email protected]

@mergify mergify bot added the component/cephfs Issues related to CephFS label Jun 1, 2022
@humblec humblec force-pushed the cephfs-attacher branch from 67a49f5 to e1ef25b Compare June 1, 2022 10:48
@humblec humblec changed the title [WIP] cephfs: remove attacher deploy: remove external-attacher sidecar from cephfs deployment Jun 1, 2022
@humblec humblec force-pushed the cephfs-attacher branch from e1ef25b to e6fdc6e Compare June 1, 2022 11:05
@nixpanic
Copy link
Member

nixpanic commented Jun 1, 2022

CephFS CSI driver dont need attacher sidecar for its operations.

It helps if you explain the why.

@nixpanic nixpanic added the component/deployment Helm chart, kubernetes templates and configuration Issues/PRs label Jun 1, 2022
@humblec humblec force-pushed the cephfs-attacher branch 3 times, most recently from 82e7fc4 to d8a7fa8 Compare June 1, 2022 11:30
@humblec
Copy link
Collaborator Author

humblec commented Jun 1, 2022

CephFS CSI driver dont need attacher sidecar for its operations.

It helps if you explain the why.

Sure . Updated the details in the PR description.

@humblec humblec requested a review from nixpanic June 1, 2022 11:50
@humblec
Copy link
Collaborator Author

humblec commented Jun 1, 2022

@Madhu-1 @nixpanic comments are addressed.. ptal.. thanks!

@humblec humblec requested a review from Madhu-1 June 1, 2022 11:53
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Jun 1, 2022

As mentioned here #1106 (comment) This was already attempted a couple of times #1106 (comment), Are we sure removing is not having any problem. Have we don't all required testing to make sure no problem occurs if we remove it?

@humblec
Copy link
Collaborator Author

humblec commented Jun 1, 2022

As mentioned here #1106 (comment) This was already attempted a couple of times #1106 (comment), Are we sure removing is not having any problem. Have we don't all required testing to make sure no problem occurs if we remove it?

@Madhu-1 As mentioned in the reply thread #1106 (comment), it should be fine to remove .. I dont expect any issues due to the same. More or less, many CSI drivers dont use this sidecar at all as it is unwanted. so we are good 👍

@humblec humblec force-pushed the cephfs-attacher branch 3 times, most recently from b388be6 to d894d2d Compare June 2, 2022 04:42
@humblec humblec added this to the release-3.7 milestone Jun 2, 2022
@humblec
Copy link
Collaborator Author

humblec commented Jun 2, 2022

/retest ci/centos/mini-e2e/k8s-1.23

1 similar comment
@humblec
Copy link
Collaborator Author

humblec commented Jun 2, 2022

/retest ci/centos/mini-e2e/k8s-1.23

Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

@humblec please squash the commits into one or 2 IMO we don't need to have many commits to for this one. and also retest can be done once PR gets approval.

@humblec humblec force-pushed the cephfs-attacher branch from d894d2d to 09c8a39 Compare June 2, 2022 14:15
@humblec
Copy link
Collaborator Author

humblec commented Jun 2, 2022

@humblec please squash the commits into one or 2 IMO we don't need to have many commits to for this one. and also retest can be done once PR gets approval.

@Madhu-1 Done.. ptal.. thanks!

@humblec humblec requested a review from Madhu-1 June 2, 2022 14:16
@humblec
Copy link
Collaborator Author

humblec commented Jun 2, 2022

/retest ci/centos/mini-e2e-helm/k8s-1.21

@humblec
Copy link
Collaborator Author

humblec commented Jun 2, 2022

/retest ci/centos/mini-e2e-helm/k8s-1.22

@humblec
Copy link
Collaborator Author

humblec commented Jun 6, 2022

@Madhu-1 comments are addresssed. ptal. thanks..

Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

There was no need to have a different commit for the Doc. ideally all changes should go into 1 or 2 commits.

Considering all the tests done as per #3149 (comment) to make sure we don't need an attacher anymore approving the PR.

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/mini-e2e-helm/k8s-1.22

@ceph-csi-bot
Copy link
Collaborator

@humblec "ci/centos/mini-e2e-helm/k8s-1.22" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2022

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@humblec
Copy link
Collaborator Author

humblec commented Jun 6, 2022

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2022

refresh

✅ Pull request refreshed

@mergify mergify bot merged commit 8d3bb82 into ceph:devel Jun 6, 2022
humblec added a commit to humblec/ceph-csi that referenced this pull request Jun 24, 2022
previously, it was a requirement to have attacher sidecar for CSI
drivers and there had an implementation of dummy mode of operation.
However skipAttach implementation has been stabilized and the dummy
mode of operation is going to be removed from the external-attacher.
Considering this driver  work on volumeattachment objects for NBD driver
use cases, we have to implement dummy controllerpublish and unpublish
and thus keep supporting our operations even in absence of dummy mode
of operation in the sidecar.

This commit make a NOOP controller publish and unpublish for RBD driver.

CephFS driver does not require attacher and it has already been made free
from the attachment operations.

Ref# ceph#3149
Ref# kubernetes-csi/external-attacher#226

Signed-off-by: Humble Chirammal <[email protected]>
humblec added a commit to humblec/ceph-csi that referenced this pull request Jul 20, 2022
previously, it was a requirement to have attacher sidecar for CSI
drivers and there had an implementation of dummy mode of operation.
However skipAttach implementation has been stabilized and the dummy
mode of operation is going to be removed from the external-attacher.
Considering this driver  work on volumeattachment objects for NBD driver
use cases, we have to implement dummy controllerpublish and unpublish
and thus keep supporting our operations even in absence of dummy mode
of operation in the sidecar.

This commit make a NOOP controller publish and unpublish for RBD driver.

CephFS driver does not require attacher and it has already been made free
from the attachment operations.

    Ref# ceph#3149
    Ref# kubernetes-csi/external-attacher#226

Signed-off-by: Humble Chirammal <[email protected]>
humblec added a commit to humblec/ceph-csi that referenced this pull request Jul 20, 2022
previously, it was a requirement to have attacher sidecar for CSI
drivers and there had an implementation of dummy mode of operation.
However skipAttach implementation has been stabilized and the dummy
mode of operation is going to be removed from the external-attacher.
Considering this driver  work on volumeattachment objects for NBD driver
use cases, we have to implement dummy controllerpublish and unpublish
and thus keep supporting our operations even in absence of dummy mode
of operation in the sidecar.

This commit make a NOOP controller publish and unpublish for RBD driver.

CephFS driver does not require attacher and it has already been made free
from the attachment operations.

    Ref# ceph#3149
    Ref# kubernetes-csi/external-attacher#226

Signed-off-by: Humble Chirammal <[email protected]>
humblec added a commit to humblec/ceph-csi that referenced this pull request Jul 20, 2022
previously, it was a requirement to have attacher sidecar for CSI
drivers and there had an implementation of dummy mode of operation.
However skipAttach implementation has been stabilized and the dummy
mode of operation is going to be removed from the external-attacher.
Considering this driver  work on volumeattachment objects for NBD driver
use cases, we have to implement dummy controllerpublish and unpublish
and thus keep supporting our operations even in absence of dummy mode
of operation in the sidecar.

This commit make a NOOP controller publish and unpublish for RBD driver.

CephFS driver does not require attacher and it has already been made free
from the attachment operations.

    Ref# ceph#3149
    Ref# kubernetes-csi/external-attacher#226

Signed-off-by: Humble Chirammal <[email protected]>
humblec added a commit to humblec/ceph-csi that referenced this pull request Jul 20, 2022
previously, it was a requirement to have attacher sidecar for CSI
drivers and there had an implementation of dummy mode of operation.
However skipAttach implementation has been stabilized and the dummy
mode of operation is going to be removed from the external-attacher.
Considering this driver  work on volumeattachment objects for NBD driver
use cases, we have to implement dummy controllerpublish and unpublish
and thus keep supporting our operations even in absence of dummy mode
of operation in the sidecar.

This commit make a NOOP controller publish and unpublish for RBD driver.

CephFS driver does not require attacher and it has already been made free
from the attachment operations.

    Ref# ceph#3149
    Ref# kubernetes-csi/external-attacher#226

Signed-off-by: Humble Chirammal <[email protected]>
humblec added a commit to humblec/ceph-csi that referenced this pull request Jul 21, 2022
previously, it was a requirement to have attacher sidecar for CSI
drivers and there had an implementation of dummy mode of operation.
However skipAttach implementation has been stabilized and the dummy
mode of operation is going to be removed from the external-attacher.
Considering this driver  work on volumeattachment objects for NBD driver
use cases, we have to implement dummy controllerpublish and unpublish
and thus keep supporting our operations even in absence of dummy mode
of operation in the sidecar.

This commit make a NOOP controller publish and unpublish for RBD driver.

CephFS driver does not require attacher and it has already been made free
from the attachment operations.

    Ref# ceph#3149
    Ref# kubernetes-csi/external-attacher#226

Signed-off-by: Humble Chirammal <[email protected]>
yati1998 added a commit to yati1998/rook that referenced this pull request Aug 2, 2022
CephFS CSI driver dont have/advertise controller publish/unpublish
capabilities, thus dont need attacher sidecar for its operations.
The presence of external-attacher adds on overhead and issues wrt
attachment in various scenarios. One of them would be the lack of
performance on syncing volumeattachment from api server..etc.
More or less we don't have controller publish and unpublish capabilities,
so we should not make use of this sidecar and cause
unnecessary addon here thus other issues.

similar changes have been added to CSI
ceph/ceph-csi#3149

Signed-off-by: yati1998 <[email protected]>
yati1998 added a commit to yati1998/rook that referenced this pull request Aug 2, 2022
CephFS CSI driver dont have/advertise controller publish/unpublish
capabilities, thus dont need attacher sidecar for its operations.
The presence of external-attacher adds on overhead and issues wrt
attachment in various scenarios. One of them would be the lack of
performance on syncing volumeattachment from api server..etc.
More or less we don't have controller publish and unpublish capabilities,
so we should not make use of this sidecar and cause
unnecessary addon here thus other issues.

similar changes have been added to CSI
ceph/ceph-csi#3149

Signed-off-by: yati1998 <[email protected]>
humblec added a commit to humblec/ceph-csi that referenced this pull request Aug 2, 2022
previously, it was a requirement to have attacher sidecar for CSI
drivers and there had an implementation of dummy mode of operation.
However skipAttach implementation has been stabilized and the dummy
mode of operation is going to be removed from the external-attacher.
Considering this driver  work on volumeattachment objects for NBD driver
use cases, we have to implement dummy controllerpublish and unpublish
and thus keep supporting our operations even in absence of dummy mode
of operation in the sidecar.

This commit make a NOOP controller publish and unpublish for RBD driver.

CephFS driver does not require attacher and it has already been made free
from the attachment operations.

    Ref# ceph#3149
    Ref# kubernetes-csi/external-attacher#226

Signed-off-by: Humble Chirammal <[email protected]>
mergify bot pushed a commit that referenced this pull request Aug 3, 2022
previously, it was a requirement to have attacher sidecar for CSI
drivers and there had an implementation of dummy mode of operation.
However skipAttach implementation has been stabilized and the dummy
mode of operation is going to be removed from the external-attacher.
Considering this driver  work on volumeattachment objects for NBD driver
use cases, we have to implement dummy controllerpublish and unpublish
and thus keep supporting our operations even in absence of dummy mode
of operation in the sidecar.

This commit make a NOOP controller publish and unpublish for RBD driver.

CephFS driver does not require attacher and it has already been made free
from the attachment operations.

    Ref# #3149
    Ref# kubernetes-csi/external-attacher#226

Signed-off-by: Humble Chirammal <[email protected]>
yati1998 added a commit to yati1998/rook that referenced this pull request Aug 3, 2022
CephFS CSI driver dont have/advertise controller publish/unpublish
capabilities, thus dont need attacher sidecar for its operations.
The presence of external-attacher adds on overhead and issues wrt
attachment in various scenarios. One of them would be the lack of
performance on syncing volumeattachment from api server..etc.
More or less we don't have controller publish and unpublish capabilities,
so we should not make use of this sidecar and cause
unnecessary addon here thus other issues.

similar changes have been added to CSI
ceph/ceph-csi#3149

Signed-off-by: yati1998 <[email protected]>
yati1998 added a commit to yati1998/rook that referenced this pull request Aug 3, 2022
CephFS CSI driver dont have/advertise controller publish/unpublish
capabilities, thus dont need attacher sidecar for its operations.
The presence of external-attacher adds on overhead and issues wrt
attachment in various scenarios. One of them would be the lack of
performance on syncing volumeattachment from api server..etc.
More or less we don't have controller publish and unpublish capabilities,
so we should not make use of this sidecar and cause
unnecessary addon here thus other issues.

similar changes have been added to CSI
ceph/ceph-csi#3149

Signed-off-by: yati1998 <[email protected]>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/ceph-csi that referenced this pull request Aug 4, 2022
previously, it was a requirement to have attacher sidecar for CSI
drivers and there had an implementation of dummy mode of operation.
However skipAttach implementation has been stabilized and the dummy
mode of operation is going to be removed from the external-attacher.
Considering this driver  work on volumeattachment objects for NBD driver
use cases, we have to implement dummy controllerpublish and unpublish
and thus keep supporting our operations even in absence of dummy mode
of operation in the sidecar.

This commit make a NOOP controller publish and unpublish for RBD driver.

CephFS driver does not require attacher and it has already been made free
from the attachment operations.

    Ref# ceph#3149
    Ref# kubernetes-csi/external-attacher#226

Signed-off-by: Humble Chirammal <[email protected]>
galexrt pushed a commit to koor-tech/koor that referenced this pull request Aug 12, 2022
CephFS CSI driver dont have/advertise controller publish/unpublish
capabilities, thus dont need attacher sidecar for its operations.
The presence of external-attacher adds on overhead and issues wrt
attachment in various scenarios. One of them would be the lack of
performance on syncing volumeattachment from api server..etc.
More or less we don't have controller publish and unpublish capabilities,
so we should not make use of this sidecar and cause
unnecessary addon here thus other issues.

similar changes have been added to CSI
ceph/ceph-csi#3149

Signed-off-by: yati1998 <[email protected]>
@thrashwerk
Copy link

I'm running a k8s 1.24 cluster and I've got a few static CephFS filesystems for which I have created static PVs and PVCs manually and use the PVCs to mount CephFS to pods.

After updating to 3.7.0 new pods just get stuck with an event Unable to attach or mount volumes: unmounted volumes=[ceph-pvc-storage1], unattached volumes=[kube-api-access-h2rfp ceph-pvc-storage1]: timed out waiting for the condition.

I don't get how things are supposed to work now. Why doesn't the CephFS PV get attached?

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Sep 13, 2022

@thrashwerk can you please open a new issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/retry/e2e Label to retry e2e retesting on approved PR's component/cephfs Issues related to CephFS component/deployment Helm chart, kubernetes templates and configuration Issues/PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants