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

Infer csi-usage based on csi-driver-availability #4039

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

0sewa0
Copy link
Contributor

@0sewa0 0sewa0 commented Nov 11, 2024

Description

DAQ-1438

Removes the following:

  • spec.oneAgent.applicationMonitoring.useCSIDriver: true/false:
    • Was used to configure if the CSI-driver should be used for application-monitoring (for injected monitored apps)
  • oneagent-readonly-host-fs feature-flag
    • Was used to configure if the CSI-driver should be used for host-monitoring and cloud-native OS agents

So the expected behavior is:

  • CSI-driver is used for applicationMonitoring IF the csi-driver was installed
    • in case the CSI-driver is missing, then the injected init-container will do the download
  • CSI-driver is required for cloudNativeFullstack
    • in case the CSI-driver is missing, the validation webhook will deny the Dl
  • CSI-driver is used for hostMonitoring IF the csi-driver was installed
    • in case the CSI-driver is missing, then the deployed OS agents will have an empty dir mounted instead.

What if I have 4 nodes, and only 2 have CSI drivers on them, and I want to use the csi-driver on the 2 nodes where it is available and not use it where it is not? (the "hybrid" configuration)

  • This scenario is NOT supported

How can this be tested?

I tested the following 3 configs, in 2 scenarios:

With CSI deployed (make deploy/helm)

a. Application monitoring -> injected apps have csi-volume

  oneAgent:
    applicationMonitoring: {}

b. Host monitoring -> deployed OS agents have csi-volume

  oneAgent:
    hostMonitoring: {}

b. Cloud Native Fullstack -> injected apps and deployed OS agents have csi-volume

  oneAgent:
    cloudNativeFullstack: {}

With CSI deployed (make deploy/helm/no-csi)

a. Application monitoring -> injected apps have NO csi-volume, does the downloading in the init-container

  oneAgent:
    applicationMonitoring: {}

b. Host monitoring -> deployed OS agents have a not-readonly host-root mount

  oneAgent:
    hostMonitoring: 
      env:
          - name: ONEAGENT_ENABLE_VOLUME_STORAGE # needed on GKE
            value: "true"

b. Cloud Native Fullstack -> validation webhook denies it

  oneAgent:
    cloudNativeFullstack: {}

@0sewa0 0sewa0 added the core Changes to core functionality of the Operator label Nov 11, 2024
@0sewa0 0sewa0 requested a review from a team as a code owner November 11, 2024 13:31
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 97.95918% with 1 line in your changes missing coverage. Please review.

Project coverage is 64.65%. Comparing base (4853ad1) to head (f6f0bca).

Files with missing lines Patch % Lines
pkg/api/validation/dynakube/oneagent.go 66.66% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4039      +/-   ##
==========================================
- Coverage   64.66%   64.65%   -0.02%     
==========================================
  Files         397      397              
  Lines       26478    26462      -16     
==========================================
- Hits        17121    17108      -13     
+ Misses       8027     8025       -2     
+ Partials     1330     1329       -1     
Flag Coverage Δ
unittests 64.65% <97.95%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

pkg/util/installconfig/modules.go Outdated Show resolved Hide resolved
pkg/api/v1beta3/dynakube/oneagent_props.go Outdated Show resolved Hide resolved

return dk.CloudNativeFullstackMode() || isAppMonitoringWithCSI || isHostMonitoringWithCSI
func (dk *DynaKube) NeedsReadOnlyOneAgents() bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we are dropping the feature flag, do we still need this check or can it just be a check if CSI driver is used in case of host agent?

Copy link
Contributor

@waodim waodim left a comment

Choose a reason for hiding this comment

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

see comment below

Co-authored-by: Albian Krasniqi <[email protected]>
Copy link
Contributor

@waodim waodim left a comment

Choose a reason for hiding this comment

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

lgtm and it also worked for me

@luhi-DT luhi-DT enabled auto-merge (squash) November 20, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Changes to core functionality of the Operator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants