Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

Fix volume class determination when binding an existing volume #1966

Merged
merged 4 commits into from
Jul 26, 2023

Conversation

g-linville
Copy link
Contributor

I found this bug while troubleshooting one of our failing tests.

Prior to this PR, if the volume class is not determined as part of the -v argument in acorn run, it would be assumed based on the label on the existing PV, and then that label would be set on the new PVC that binds the PV. However, the code fails to properly set the storageClassName spec field on the PVC, and it takes the default storage class, leading to a possible mismatch and the inability to actually bind the volume. This change makes sure that the storage class is set based on the volume class under all circumstances.

Checklist

  • The title of this PR would make a good line in Acorn's Release Note's Changelog
  • The title of this PR ends with a link to the main issue being address in parentheses, like: This is a title (#1216). Here's an example
  • All relevant issues are referenced in the PR description. NOTE: don't use GitHub keywords that auto-close issues
  • Commits follow contributing guidance
  • Automated tests added to cover the changes. If tests couldn't be added, an explanation is provided in the Verification and Testing section
  • Changes to user-facing functionality, API, CLI, and upgrade impacts are clearly called out in PR description
  • PR has at least two approvals before merging (or a reasonable exception, like it's just a docs change)

(Our e2e tests cover this.)

Signed-off-by: Grant Linville <[email protected]>
Signed-off-by: Grant Linville <[email protected]>
@g-linville g-linville merged commit 65b88d8 into acorn-io:main Jul 26, 2023
@g-linville g-linville deleted the volume-binding-class-fix branch July 26, 2023 03:14
@sangee2004
Copy link
Contributor

Tested with acorn version - v0.8.0-alpha7-176-g941a4494

  1. Deploy Apps with volume
args: {
fileName: "hello.html"
fileContent: "content"
}
containers: {
    frontend: {
        build: "."
        cmd: ["/root/start.sh", args.fileContent,args.fileName]
        dirs: {
            "/scratch": "volume://scratch-data"
        }
        sidecars: {
            sidecar: {
            ports: publish: "80/http"
            image: "nginx"
            dirs: {
                "/usr/share/nginx/html/test1": "volume://scratch-data"
            }
          }
        }
}
}
volumes:
{
"scratch-data": {}
}
  1. Once app is deployed successfully, delete app (with out removing volumes). Volume gets to released state.
acorn volumes
NAME                     BOUND-VOLUME   CAPACITY   VOLUME-CLASS   STATUS     ACCESS-MODES   CREATED
mytestvol.scratch-data   scratch-data   10Gi       ebs-retain     released   RWO            9m31s ago
  1. Deploy another apps that binds this existing volume using acorn run -n mytestvolbind --volume mytestvol.scratch-data:my-data -f Acornfilebind

Acornfilebind

containers: {
mynginx: {
ports: publish: "80/http"
image: "nginx"
dirs: {
"/usr/share/nginx/html/test1": "volume://my-data"
}
}
}

App gets deployed successfully and bound to the existing volume (from step2) .

acorn volumes
NAME                     BOUND-VOLUME   CAPACITY   VOLUME-CLASS   STATUS    ACCESS-MODES   CREATED
mytestvol.scratch-data   my-data-bind   10Gi       ebs-retain     bound     RWO            10m ago

Able to access contents of the volume successfully from the app.

@cjellick cjellick removed this from the v0.8.0 milestone Sep 5, 2023
cloudnautique pushed a commit to cloudnautique/runtime that referenced this pull request Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants