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

[JENKINS-47591] dynamic pvc workspace volume #600

Merged
merged 2 commits into from Sep 27, 2019

Conversation

@runzexia
Copy link
Member

runzexia commented Sep 20, 2019

Signed-off-by: runzexia runzexia@yunify.com
Add this kind of workspace volume because during use, we found that some IO-intensive tasks have some problems with the performance of emptyDir.
I want to use dynamic pvc to avoid this performance problem.

When I implemented this part, I found that the original WorkspaceVolume abstract class seems to be only suitable for static PVC.
So I want to find some comments about this part of the code, any comments are very grateful

@Vlatombe @jglick

@carlossg carlossg changed the title [WIP| dynamic pvc workspace volume [WIP][JENKINS-47591] dynamic pvc workspace volume Sep 20, 2019
@carlossg

This comment has been minimized.

Copy link
Contributor

carlossg commented Sep 20, 2019

The interesting places where this gets used is

I guess you can ignore the volume name and set some random name. Or could add the podName to the arguments, so you can name the volume podName-xxxx

You would need to clean up the PVCs after the builds too, I think it would be possible to just set the owner reference in the PVC and let k8s delete the pvc when the pod is deleted
https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/#owners-and-dependents

There is a Jira at https://issues.jenkins-ci.org/browse/JENKINS-47591

@runzexia

This comment has been minimized.

Copy link
Member Author

runzexia commented Sep 20, 2019

The interesting places where this gets used is

I guess you can ignore the volume name and set some random name. Or could add the podName to the arguments, so you can name the volume podName-xxxx

You would need to clean up the PVCs after the builds too, I think it would be possible to just set the owner reference in the PVC and let k8s delete the pvc when the pod is deleted
https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/#owners-and-dependents

There is a Jira at https://issues.jenkins-ci.org/browse/JENKINS-47591

Very useful guide, thanks! ! I will try it recently.

@runzexia runzexia changed the title [WIP][JENKINS-47591] dynamic pvc workspace volume [JENKINS-47591] dynamic pvc workspace volume Sep 24, 2019
@runzexia runzexia force-pushed the runzexia:dynamic-pvc branch from 1e48db9 to 3cd6e12 Sep 24, 2019
@runzexia

This comment has been minimized.

Copy link
Member Author

runzexia commented Sep 24, 2019

@carlossg @Vlatombe @jglick ready for review, ci failed with timeout, need permission to create PVC?

@Vlatombe

This comment has been minimized.

Copy link
Member

Vlatombe commented Sep 24, 2019

@runzexia the test can be guarded similarly to

try {
cloud.connect().apps().deployments().withName("cascading-delete").delete();
} catch (KubernetesClientException x) {
// Failure executing: DELETE at: https://…/apis/apps/v1/namespaces/kubernetes-plugin-test/deployments/cascading-delete. Message: Forbidden!Configured service account doesn't have access. Service account may have been revoked. deployments.apps "cascading-delete" is forbidden: User "system:serviceaccount:…:…" cannot delete resource "deployments" in API group "apps" in the namespace "kubernetes-plugin-test".
assumeNoException("was not permitted to clean up any previous deployment, so presumably cannot run test either", x);
}

@runzexia runzexia force-pushed the runzexia:dynamic-pvc branch from a3fac5b to a95068f Sep 24, 2019
@runzexia

This comment has been minimized.

Copy link
Member Author

runzexia commented Sep 24, 2019

the test can be guarded similarly to

@Vlatombe
I think this method is a resource for handling conflict names?
The createPvc method is created from the pod name and now includes cascading deletes in the code.
IMHO, the test timeout is because there is no permission to create pvc now, causing the agent to never start.

@runzexia runzexia requested a review from Vlatombe Sep 25, 2019
Copy link
Member

Vlatombe left a comment

I added a guard block to skip the new test if the sa doesn't have the right to handle pvcs.

Signed-off-by: runzexia <runzexia@yunify.com>
@runzexia runzexia force-pushed the runzexia:dynamic-pvc branch from bbb71f8 to 7f12088 Sep 25, 2019
@runzexia runzexia requested a review from Vlatombe Sep 25, 2019
@Vlatombe

This comment has been minimized.

Copy link
Member

Vlatombe commented Sep 25, 2019

Please avoid force-pushes, it breaks incremental review.

@runzexia

This comment has been minimized.

Copy link
Member Author

runzexia commented Sep 25, 2019

Ok, I will avoid this in the future.
By the way, I would like to ask if there is a plan to draft a release after this PR is merged?

Signed-off-by: runzexia <runzexia@yunify.com>
@Vlatombe

This comment has been minimized.

Copy link
Member

Vlatombe commented Sep 25, 2019

Yes, I'll cut a release

@runzexia runzexia requested a review from Vlatombe Sep 25, 2019
Copy link
Member

Vlatombe left a comment

LGTM.

@jglick any more comments?

@Vlatombe Vlatombe merged commit 26be9fd into jenkinsci:master Sep 27, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
@tarioch

This comment has been minimized.

Copy link

tarioch commented Oct 4, 2019

Is there a way to configure RequestsSize and StorageClassName through the UI? I don't seem to be able to choose/enter those values when selecting the "Dynamic Persistent Volume Claim Workspace Volume".

What would be needed to expose those two settings through the UI?

Then for doing a PR for the jenkins helm chart to include those permissions in the RBAC setup, would it just be the resource "persistentvolumeclaims"?

@runzexia

This comment has been minimized.

Copy link
Member Author

runzexia commented Oct 6, 2019

have created a pull request
#614
@tarioch

@runzexia runzexia deleted the runzexia:dynamic-pvc branch Oct 6, 2019
@runzexia runzexia mentioned this pull request Oct 9, 2019
@pavenova

This comment has been minimized.

Copy link

pavenova commented Oct 9, 2019

@runzexia
Hi, I can see PR was merged, so new version was released - 1.19
But in the jenkins I am able still see as latest 1.16.7

Can you please double checked, if you did not forgot eg. to change the plugin version or something?

Thank you :)

@Vlatombe

This comment has been minimized.

Copy link
Member

Vlatombe commented Oct 9, 2019

@pavenova Please update your Jenkins instance, you probably have a core that is too old. We generally bump the required Jenkins core version when bumping the second digit (1.16 -> 1.17)

@pavenova

This comment has been minimized.

Copy link

pavenova commented Oct 9, 2019

@pavenova Please update your Jenkins instance, you probably have a core that is too old. We generally bump the required Jenkins core version when bumping the second digit (1.16 -> 1.17)

Hello Vicent, thanks for the point,
I have ver. 2.164.3 LTS , and new version of plugin did not appeared (of course the plugin list was successfully refreshed), so from the comment I guess jenkins version should be sufficient

@Vlatombe

This comment has been minimized.

Copy link
Member

Vlatombe commented Oct 9, 2019

@pavenova Please refer to release notes for the required core version. Latest version of the plugin requires core 2.176.1 or later.

@pavenova

This comment has been minimized.

Copy link

pavenova commented Oct 9, 2019

@pavenova Please refer to release notes for the required core version. Latest version of the plugin requires core 2.176.1 or later.

Ahaah, got it, thank you :)

@AustinReichert

This comment has been minimized.

Copy link

AustinReichert commented Oct 11, 2019

I'm testing this new addition for Dynamic PVCs on 2.190.1, Kubernetes Plugin version 1.20.0, the PV and PVCs are properly being created and disappearing, but I'm seeing this when the container attempts to start:

Warning: JnlpProtocol3 is disabled by default, use JNLP_PROTOCOL_OPTS to alter the behavior
Oct 11, 2019 11:20:18 PM hudson.remoting.jnlp.Main createEngine
INFO: Setting up agent: default-dn816
Oct 11, 2019 11:20:18 PM hudson.remoting.jnlp.Main$CuiListener <init>
INFO: Jenkins agent is running in headless mode.
Oct 11, 2019 11:20:18 PM hudson.remoting.Engine startEngine
INFO: Using Remoting version: 3.29
Exception in thread "main" java.io.IOException: The specified working directory should be fully accessible to the remoting executable (RWX): /home/jenkins/agent
        at org.jenkinsci.remoting.engine.WorkDirManager.verifyDirectory(WorkDirManager.java:249)
        at org.jenkinsci.remoting.engine.WorkDirManager.initializeWorkDir(WorkDirManager.java:202)
        at hudson.remoting.Engine.startEngine(Engine.java:251)
        at hudson.remoting.Engine.startEngine(Engine.java:227)
        at hudson.remoting.jnlp.Main.main(Main.java:228)
        at hudson.remoting.jnlp.Main._main(Main.java:223)
        at hudson.remoting.jnlp.Main.main(Main.java:189)

I'm using Ceph RBD with a ReadWriteOnce and the mainline Jenkins Helm chart with JCasC 1.32. Wondering if this is mounting as "root" or something. Let me know if you need more info. Any thoughts?

@runzexia

This comment has been minimized.

Copy link
Member Author

runzexia commented Oct 14, 2019

@AustinReichert
try set

  securityContext:
    runAsUser:0
    fsGroup: 0

in yaml

@Vlatombe

This comment has been minimized.

Copy link
Member

Vlatombe commented Oct 14, 2019

Or just

securityContext:
  fsGroup: 1000
@tarioch

This comment has been minimized.

Copy link

tarioch commented Oct 15, 2019

Created a PR for the jenkins helm chart helm/charts#17973

@DonAndrey

This comment has been minimized.

Copy link

DonAndrey commented Oct 17, 2019

Hello, currently I'm trying to set up a new jenkins on kubernetes, but I am facing some problems with the new dynamic pvc provision. You can see the error that it fires in the following image:

Capture

Any help would be really helpful!

@Vlatombe

This comment has been minimized.

Copy link
Member

Vlatombe commented Oct 17, 2019

@DonAndrey It says the size you provided is invalid.

@Vlatombe

This comment has been minimized.

Copy link
Member

Vlatombe commented Oct 17, 2019

@DonAndrey nvm. Revert to an older version of the plugin until this is fixed.

@DonAndrey

This comment has been minimized.

Copy link

DonAndrey commented Oct 17, 2019

Thanks @Vlatombe !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.