-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix pvc vs bucket detection case 🌵 #475
Fix pvc vs bucket detection case 🌵 #475
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
+1 from me! |
/hold I'll be adding some unit tests 👼 |
ab0587d
to
190ac5f
Compare
/hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this @vdemeester !! just a couple comments about logging/error handling :D
if strings.TrimSpace(location) == "" { | ||
return true | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel like in some of these cases we should be logging a warning or something, what do you think? if the user provided the configmap but didn't provide the data or url, that seems like they made a mistake? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobcatfish good point, I'll add some logging in there 😉
pkg/artifacts/artifacts_storage.go
Outdated
@@ -68,11 +54,31 @@ func InitializeArtifactStorage(pr *v1alpha1.PipelineRun, c kubernetes.Interface) | |||
return NewArtifactBucketConfigFromConfigMap(configMap) | |||
} | |||
|
|||
func needsPVC(configMap *corev1.ConfigMap, err error) bool { | |||
if err != nil { | |||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm in this error case we should definitely at least log i think (unless it's a not found error, which we can probably check for explicitly) - if not even fail the run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point 👍 didn't change the current behavior but if it's not a not found error, it seems fishy and we may fail the run as a fail early
👼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch. Looks like @bobcatfish has comments so I will leave this upto her to lgtm this PR.
I had a thought when @vdemeester was debugging this. From a user how can I realize from pod spec whether this is using pvc or bucket?
If its bucket storage then pod spec should contain the volume mount pointing to config map and if its PVC then it should have pvc volume mount. The information exists but it is in gigantic pod spec of taskrun.
Can we use labels or annotation to add some meta info to get this info in clear way? Do you think this would have helped you debug faster or better @vdemeester
/assign @bobcatfish |
Depending on the `config-artifact-bucket` configMap, `build-pipeline` will create either a PVC or GCS Bucket for sharing output between task(run)s in the pipeline. There was a bug (or missing case) in the code that checks if it needs to create PVC. We want to create a PVC in the following cases: - the `config-artifact-bucket` configMap is missing - the `config-artifact-bucket` configMap data is empty - the `config-artifact-bucket` configMap has no `BucketLocationKey` (aka `location`) *or* it's value is empty. The 2nd case wasn't correctly handled between `InitializedArtifactStorage` and `GetArtifactStorage` functions. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
48f9141
to
dba7b6b
Compare
test/e2e-tests-yaml.sh
Outdated
# - https://github.com/knative/build-pipeline/pull/443 | ||
# for test in taskrun pipelinerun; do | ||
for test in taskrun; do | ||
for test in taskrun pipelinerun; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobcatfish trying something 🙃 related to #477
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
ok it's trying to copy |
dba7b6b
to
c8f5430
Compare
Removed dba7b6b commit 👼 |
The following is the coverage report on pkg/.
|
/lgtm |
In response to this:
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/test-infra repository. |
Also, because why not: |
In response to this:
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/test-infra repository. |
ooo boxes, that's a good one @abayer |
Depending on the
config-artifact-bucket
configMap,build-pipeline
will create either a PVC or GCS Bucket for sharing output between
task(run)s in the pipeline. There was a bug (or missing case) in the
code that checks if it needs to create PVC.
We want to create a PVC in the following cases:
config-artifact-bucket
configMap is missingconfig-artifact-bucket
configMap data is emptyconfig-artifact-bucket
configMap has noBucketLocationKey
(akalocation
) or it's value is empty.The 2nd case wasn't correctly handled between
InitializedArtifactStorage
andGetArtifactStorage
functions./cc @shashwathi
Signed-off-by: Vincent Demeester vdemeest@redhat.com