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

fix(usage): add nil checks to avoid panic #1720

Merged
merged 3 commits into from
Jul 1, 2020
Merged

fix(usage): add nil checks to avoid panic #1720

merged 3 commits into from
Jul 1, 2020

Conversation

kmova
Copy link
Contributor

@kmova kmova commented Jun 16, 2020

Signed-off-by: kmova kiran.mova@mayadata.io

Why is this PR required? What issue does it fix?:
openebs/openebs#3064

What this PR does?:
Adds nil checks to catch provisioning errors.

Does this PR require any upgrade changes?:
No

If the changes in this PR are manually verified, list down the scenarios covered::

  • Verified by creating a PVC with name > 63 chars long. The output shows the following error when kubectl describe pvc is executed.

    Warning  ProvisioningFailed    2s (x2 over 17s)   openebs.io/provisioner-iscsi_openebs-provisioner-684b6b8584-96kvx_261d65dd-b150-11ea-bd60-2e484c01cabe  failed to provision volume with StorageClass "openebs-jiva-default": Internal Server Error: failed to create volume
     ....
     .... snipped verbose output. 
     ....
     failed to create service: Service "pvc-f872ca1d-4bf4-453f-805f-e2fd3ed31003-ctrl-svc" is invalid: metadata.labels: Invalid value: "jiva-pvc-more-than-63-chars-long-due-helm-sub-chart-insallation-or-some-crazy-logic": must be no more than 63 characters
    
  • Verified by creating a PVC with name < 63 chars long. Volume gets created successfully.

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

  • Fixes #
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

@kmova kmova added pr/hold-merge Hold on the merge to complete few activities. and removed pr/hold-merge Hold on the merge to complete few activities. labels Jun 16, 2020
Copy link
Contributor

@shubham14bajpai shubham14bajpai left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -85,7 +85,10 @@ func (s *HTTPServer) volumeV1alpha1SpecificRequest(resp http.ResponseWriter, req
switch req.Method {
case "POST":
cvol, err := volOp.create()
pvc := cvol.ObjectMeta.Labels["openebs.io/persistent-volume-claim"]
pvc := ""
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to pvcName ?

@kmova kmova added the pr/hold-merge Hold on the merge to complete few activities. label Jun 19, 2020
Copy link
Contributor

@prateekpandey14 prateekpandey14 left a comment

Choose a reason for hiding this comment

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

lgtm

kmova added 3 commits July 1, 2020 04:37
Once a PVC create request is processed by the CAST,
the response is inspected to fetch the PVC name
and other details to generate an analytics log.

This was done as part of the PR: #1708

This commit adds a nil check to handle the case of error response
to avoid a panic.

Signed-off-by: kmova <kiran.mova@mayadata.io>
Signed-off-by: kmova <kiran.mova@mayadata.io>
Signed-off-by: kmova <kiran.mova@mayadata.io>
Copy link

@pawanpraka1 pawanpraka1 left a comment

Choose a reason for hiding this comment

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

looks good.

@kmova kmova removed the pr/hold-merge Hold on the merge to complete few activities. label Jul 1, 2020
@kmova kmova merged commit d70090d into openebs-archive:master Jul 1, 2020
kmova added a commit that referenced this pull request Jul 1, 2020
Once a PVC create request is processed by the CAST,
the response is inspected to fetch the PVC name
and other details to generate an analytics log.

This was done as part of the PR: #1708

This commit adds a nil check to handle the case of an error response
to avoid a panic.

Signed-off-by: kmova <kiran.mova@mayadata.io>

(cherry picked from commit d70090d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants