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

PWX-32428,PWX-27672: shared mounts fix for PKS and privileged:false #1181

Merged
merged 3 commits into from
Aug 4, 2023

Conversation

zoxpx
Copy link
Collaborator

@zoxpx zoxpx commented Aug 3, 2023

  • fixes shared mounts for PKS (PWX-32428, regression introduced via PWX-31842)
  • fixed shared mounts for privileged:false (PWX-27672)

What this PR does / why we need it:

  • PWX-32428: Fixes regression introduced w/ PWX-31842
  • PWX-27672: Fixed shared/sharedV4 volumes when privileged:false annotation used
  • new limitation: portworx.io/privileged: true/false annotation cannot be used unless on px-3.0.1 (or higher)
  • new pksVolumeInfo.mountPropagation tweak can override volume's propagation (value - removes propagation setting)

FIX:

  • we are taking advantage of the new mounts-rules changes introduced w/ px-3.0.1
  • removing the :shared flag for -v /var/lib/osd:/var/lib/osd:shared
  • adding new -v /var/lib/osd/pxns:/var/lib/osd/pxns:shared and -v /var/lib/osd/mounts:/var/lib/osd/mounts:shared mounts

Which issue(s) this PR fixes (optional)
Closes # PWX-32428
Also follow-up fix for PWX-27672

Special notes for your reviewer:

note: using https://github.com/libopenstorage/operator/tree/PWX-27672_privileged_switch_for_oci-mons as a base-branch

@zoxpx
Copy link
Collaborator Author

zoxpx commented Aug 3, 2023

This is odd, unrelated TestMonitoringMetricsEnabled started failing on Travis-checks (this PR did not modify that part of the code)

  • please ignore for now, I'll recheck everything once base-branch is committed, and this PR rebased off of master-branch

@pureneelesh
Copy link
Contributor

sharedv4 mount changes look good to me

@zoxpx
Copy link
Collaborator Author

zoxpx commented Aug 4, 2023

NOTE -- here's the result of the actual test on a PKS system:

  • deployment on px-3.0.0
    • note, we're using same mountpoints as before
kind: Pod
spec:
  containers:
  - name: portworx
    image: docker.io/portworx/oci-monitor:3.0.0
    volumeMounts:
    - mountPath: /var/lib/osd/log
      name: pxlogs
    - mountPath: /var/lib/osd
      mountPropagation: Bidirectional
      name: varlibosd
  volumes:
  - hostPath:
      path: /var/vcap/store/lib/osd/log
      type: ""
    name: pxlogs
  - hostPath:
      path: /var/lib/osd
      type: ""
    name: varlibosd
  • deployment on px-4.0.0 (a.k.a. master)
    • note, using non-shared /var/lib/osd and 2 extra shared mountpoint via params
kind: Pod
spec:
  containers:
  - name: portworx
    image: docker.io/zoxpx/oci-monitor:4.0.0-dev
    args:
    - ...
    - -v
    - /var/lib/osd/pxns:/var/lib/osd/pxns:shared
    - -v
    - /var/lib/osd/mounts:/var/lib/osd/mounts:shared
    volumeMounts:
    - mountPath: /var/lib/osd
      name: varlibosd
  volumes:
  - hostPath:
      path: /var/vcap/store/lib/osd
      type: ""
    name: varlibosd

Base automatically changed from PWX-27672_privileged_switch_for_oci-mons to master August 4, 2023 07:09
* Portworx PODs (oci-monitors) no longer using `privileged:true` - but using 5 capabilities instead
* adding new `STC.security.privileged = true` switch, to roll back to the original `privileged:true` security-setting
* we're also disabling all Bidirectional-mounts, and converting them to regular-mounts  (warning will indicate to change STC...privileged=true to enable back)

Signed-off-by: Zoran Rajic <zrajic@purestorage.com>
Signed-off-by: Zoran Rajic <zox@portworx.com>
* fixes shared mounts for PKS (PWX-32428, regression introduced via PWX-31842)
* fixed shared mounts for privileged:false (PWX-27672)

Signed-off-by: Zoran Rajic <zox@portworx.com>
@zoxpx zoxpx force-pushed the PWX-32428,PWX-27672_fix_shared_mounts branch from b11fcc8 to 1527ed7 Compare August 4, 2023 07:41
@zoxpx
Copy link
Collaborator Author

zoxpx commented Aug 4, 2023

note, code rebased off of latest master

zoxpx added a commit that referenced this pull request Aug 4, 2023
…1181)

* fixes shared mounts for PKS (PWX-32428, regression introduced via PWX-31842)
* fixed shared mounts for privileged:false (PWX-27672)

Signed-off-by: Zoran Rajic <zox@portworx.com>
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Patch coverage: 95.83% and project coverage change: +0.05% 🎉

Comparison is base (852a14d) 75.79% compared to head (1527ed7) 75.84%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1181      +/-   ##
==========================================
+ Coverage   75.79%   75.84%   +0.05%     
==========================================
  Files          64       64              
  Lines       18123    18152      +29     
==========================================
+ Hits        13736    13768      +32     
+ Misses       3411     3408       -3     
  Partials      976      976              
Files Changed Coverage Δ
pkg/migration/generate.go 88.82% <ø> (ø)
drivers/storage/portworx/deployment.go 95.90% <95.83%> (+0.33%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zoxpx
Copy link
Collaborator Author

zoxpx commented Aug 4, 2023

Thanks for the review Neelesh and Piyush -- merging the PR, will cherry-pick into px-rel-23.7.0 via #1185

@zoxpx zoxpx merged commit e42992a into master Aug 4, 2023
8 checks passed
@zoxpx zoxpx deleted the PWX-32428,PWX-27672_fix_shared_mounts branch August 4, 2023 08:46
zoxpx added a commit that referenced this pull request Aug 4, 2023
* PWX-27672: Reducing Privileged-requirements for Portworx PODs (#1141)

New functionality triggered via `portworx.io/privileged = false` annotation:
* Portworx PODs (oci-monitors) no longer using `privileged:true` - but using 5 capabilities instead
* we're also disabling all Bidirectional-mounts, and converting them to regular-mounts  (warning will indicate to change STC...privileged=true to enable back)

Signed-off-by: Zoran Rajic <zox@portworx.com>

Manually fixed Conflicts:
        drivers/storage/portworx/util/util.go
        pkg/controller/storagecluster/storagecluster.go

* PWX-32428,PWX-27672: shared mounts fix for PKS and privileged:false (#1181)

* fixes shared mounts for PKS (PWX-32428, regression introduced via PWX-31842)
* fixed shared mounts for privileged:false (PWX-27672)

Signed-off-by: Zoran Rajic <zox@portworx.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants