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

OADP-4379: read ephemeral-storage in code #1477

Conversation

mateusoliveira43
Copy link
Contributor

@mateusoliveira43 mateusoliveira43 commented Jul 25, 2024

Why the changes were made

Users were able to set ephemeral-storage values in DPA, but OADP code would never read the values. This PR fixes this behavior.

How to test the changes made

Create OADP with make deploy-olm and create DPA with ephemeral-storage values. Check if values are passed to Velero and node-agent containers.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 25, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 25, 2024

@mateusoliveira43: This pull request references OADP-4379 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.17.0" version, but no target version was set.

In response to this:

Why the changes were made

Users were able to set storage and ephemeral-storage values in DPA, but OADP code would never read the values. This PR fixes this behavior.

How to test the changes made

Create OADP with make deploy-olm and create DPA with storage and ephemeral-storage values. Check if values are passed to Velero and node-agent containers.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 25, 2024
Copy link

openshift-ci bot commented Jul 25, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2024
@mateusoliveira43 mateusoliveira43 force-pushed the fix/read-ephemeral-storage-in-code branch from f384f1f to 3f29ccc Compare July 31, 2024 17:02
@mateusoliveira43

This comment was marked as resolved.

@mateusoliveira43 mateusoliveira43 force-pushed the fix/read-ephemeral-storage-in-code branch from 13d53de to e49d32d Compare August 2, 2024 18:08
@mateusoliveira43 mateusoliveira43 force-pushed the fix/read-ephemeral-storage-in-code branch from 37e3102 to 1015570 Compare August 2, 2024 21:40
@mateusoliveira43 mateusoliveira43 marked this pull request as ready for review August 2, 2024 21:40
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2024
@openshift-ci openshift-ci bot requested review from kaovilai and mpryc August 2, 2024 21:41
controllers/velero.go Outdated Show resolved Hide resolved
controllers/velero.go Outdated Show resolved Hide resolved
controllers/velero.go Outdated Show resolved Hide resolved
controllers/velero.go Show resolved Hide resolved
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 8, 2024

@mateusoliveira43: This pull request references OADP-4379 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.17.0" version, but no target version was set.

In response to this:

Why the changes were made

Users were able to set ephemeral-storage values in DPA, but OADP code would never read the values. This PR fixes this behavior.

How to test the changes made

Create OADP with make deploy-olm and create DPA with ephemeral-storage values. Check if values are passed to Velero and node-agent containers.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@mateusoliveira43 mateusoliveira43 force-pushed the fix/read-ephemeral-storage-in-code branch from 1015570 to 25b9d0b Compare August 8, 2024 18:02
@mateusoliveira43
Copy link
Contributor Author

/retest

@weshayutin
Copy link
Contributor

hrm.. shouldn't the crd spec be updated or we can't because it's upstream only?
Should see ephemeral storage

 oc explain dpa.spec.configuration.nodeAgent.podConfig.resourceAllocations
KIND:     DataProtectionApplication
VERSION:  oadp.openshift.io/v1alpha1

DESCRIPTION:
     resourceAllocations defines the CPU and Memory resource allocations for the
     Pod

If we can't update the crd doc, we should clone the bug for a doc add to our downstream only.

@shubham-pampattiwar
Copy link
Member

hrm.. shouldn't the crd spec be updated or we can't because it's upstream only? Should see ephemeral storage

 oc explain dpa.spec.configuration.nodeAgent.podConfig.resourceAllocations
KIND:     DataProtectionApplication
VERSION:  oadp.openshift.io/v1alpha1

DESCRIPTION:
     resourceAllocations defines the CPU and Memory resource allocations for the
     Pod

If we can't update the crd doc, we should clone the bug for a doc add to our downstream only.

@weshayutin Good catch ! We own the DPA CRD, we should be able to update the description here via kube builder markers.

@weshayutin
Copy link
Contributor

  configuration:
    nodeAgent:
      enable: true
      podConfig:
        resourceAllocations:
          requests:
            cpu: 200m
            ephemeral-storage: 5G
            memory: 256Mi
      uploaderType: kopia
    velero:
      defaultPlugins:
        - kubevirt
        - csi
        - openshift
        - aws
      podConfig:
        resourceAllocations:
          requests:
            cpu: 200m
            ephemeral-storage: 5G
            memory: 256Mi
 oc get daemonsets.apps node-agent -o yaml | grep ephemeral
            ephemeral-storage: 5G

Needs a doc update in the crd for the dpa for approval

@mateusoliveira43 mateusoliveira43 force-pushed the fix/read-ephemeral-storage-in-code branch from 25b9d0b to 2b44549 Compare August 12, 2024 21:48
Copy link
Member

@shubham-pampattiwar shubham-pampattiwar left a comment

Choose a reason for hiding this comment

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

PR changes worked for me:

    ports:
    - containerPort: 8085
      name: metrics
      protocol: TCP
    resources:
      requests:
        cpu: 512m
        ephemeral-storage: 2Gi
        memory: 1Gi
    securityContext:
      allowPrivilegeEscalation: false
--
  initContainers:
  - image: quay.io/konveyor/openshift-velero-plugin:latest
    imagePullPolicy: Always
    name: openshift-velero-plugin
    resources:
      requests:
        cpu: 512m
        ephemeral-storage: 2Gi
        memory: 1Gi
    securityContext:
      allowPrivilegeEscalation: false
--
      readOnly: true
  - image: quay.io/konveyor/velero-plugin-for-aws:latest
    imagePullPolicy: Always
    name: velero-plugin-for-aws
    resources:
      requests:
        cpu: 512m
        ephemeral-storage: 2Gi
        memory: 1Gi
    securityContext:
      allowPrivilegeEscalation: false

Requested one change: https://github.com/openshift/oadp-operator/pull/1477/files#r1714703057

@weshayutin
Copy link
Contributor

/test 4.16-e2e-test-aws

@kaovilai
Copy link
Member

need rebase

@mateusoliveira43 mateusoliveira43 force-pushed the fix/read-ephemeral-storage-in-code branch from 559a154 to 1caa8d2 Compare August 13, 2024 19:29
Comment on lines -45 to -47
fsPvHostPath = getFsPvHostPath("")
pluginsHostPath = getPluginsHostPath("")

Copy link
Member

Choose a reason for hiding this comment

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

@shubham-pampattiwar can you review if this is ok? you recently modified this in here

Choose a reason for hiding this comment

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

yeah we need those

Copy link
Contributor Author

@mateusoliveira43 mateusoliveira43 Aug 13, 2024

Choose a reason for hiding this comment

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

removed usage in my changes

Copy link

openshift-ci bot commented Aug 13, 2024

@mateusoliveira43: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Copy link
Contributor

@weshayutin weshayutin 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 openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2024
Copy link

openshift-ci bot commented Aug 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mateusoliveira43, weshayutin

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-merge-bot openshift-merge-bot bot merged commit b51ce21 into openshift:master Aug 14, 2024
15 checks passed
@mateusoliveira43
Copy link
Contributor Author

/cherry-pick oadp-1.4

@openshift-cherrypick-robot
Copy link
Contributor

@mateusoliveira43: #1477 failed to apply on top of branch "oadp-1.4":

Applying: fix: read ephemeral-storage in code
Using index info to reconstruct a base tree...
M	api/v1alpha1/oadp_types.go
M	bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml
M	config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml
M	controllers/nodeagent.go
M	controllers/nodeagent_test.go
M	controllers/validator.go
M	controllers/velero.go
M	controllers/velero_test.go
M	pkg/credentials/credentials.go
M	pkg/credentials/credentials_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/credentials/credentials_test.go
Auto-merging pkg/credentials/credentials.go
Auto-merging controllers/velero_test.go
CONFLICT (content): Merge conflict in controllers/velero_test.go
Auto-merging controllers/velero.go
Auto-merging controllers/validator.go
CONFLICT (content): Merge conflict in controllers/validator.go
Auto-merging controllers/nodeagent_test.go
CONFLICT (content): Merge conflict in controllers/nodeagent_test.go
Auto-merging controllers/nodeagent.go
Auto-merging config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml
Auto-merging bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml
Auto-merging api/v1alpha1/oadp_types.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix: read ephemeral-storage in code
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick oadp-1.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

mateusoliveira43 added a commit to mateusoliveira43/oadp-operator that referenced this pull request Aug 14, 2024
Signed-off-by: Mateus Oliveira <[email protected]>
(cherry picked from commit b51ce21)
openshift-merge-bot bot pushed a commit that referenced this pull request Aug 15, 2024
Signed-off-by: Mateus Oliveira <[email protected]>
(cherry picked from commit b51ce21)
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. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants