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.

Sign up for GitHub

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

Jump to bottom

feat: support artifact retention #1408

Merged
merged 3 commits into from
Apr 15, 2020
Merged

feat: support artifact retention #1408

merged 3 commits into from
Apr 15, 2020

Conversation

zhujian7
Copy link
Collaborator

@zhujian7 zhujian7 commented Apr 9, 2020 •
edited
Loading

What this PR does / why we need it:

  • artifact retention
  • workflowRun dry-run, check whether artifacts disk space enough for now.

Which issue(s) this PR is related to (optional, link to 3rd issue(s)):

Fixes #

Reference to #

Special notes for your reviewer:

/cc @supereagle @hyy0322

Release note:

NONE

@caicloud-bot caicloud-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 9, 2020
@zhujian7 zhujian7 changed the title wip:feat: support artifact retention feat: support artifact retention Apr 14, 2020
@caicloud-bot caicloud-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 14, 2020
@hyy0322
Copy link
Contributor

hyy0322 commented Apr 14, 2020

lgtm

supereagle
supereagle reviewed Apr 15, 2020
View reviewed changes
go.mod
sigs.k8s.io/yaml v1.2.0 // indirect
)

replace (
Copy link
Member

Choose a reason for hiding this comment

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

Need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, since we used k8s.io/kubernetes

supereagle
supereagle reviewed Apr 15, 2020
View reviewed changes
helm/cyclone/templates/server.yaml Outdated
Comment on lines 62 to 63
"retention_seconds": {{ .Values.server.artifact.retentionSeconds }},
"available_disk_percentage": {{ .Values.server.artifact.availableDiskPercentage }}
Copy link
Member

Choose a reason for hiding this comment

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

It will be better to use similar style and extensible names.

Suggested change
"retention_seconds": {{ .Values.server.artifact.retentionSeconds }},
"available_disk_percentage": {{ .Values.server.artifact.availableDiskPercentage }}
"retention_interval": {{ .Values.server.artifact.retentionInterval }},
"retention_threshold": {{ .Values.server.artifact.retentionThreshold }}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will change available_disk_percentage to retention_disk_protection_threshold

release/cyclone-server.yaml Outdated
@@ -64,8 +64,8 @@ _config:
modes:
- ReadWriteMany
storage:
request: 200Gi
limit: 200Gi
request: 500Gi
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the quota is too large as we will not allow oversold of storage.
/cc @iawia002

Copy link

Choose a reason for hiding this comment

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

/hold
We are going to discuss this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

update it to 10Gi

Copy link

Choose a reason for hiding this comment

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

LGTM

pkg/server/handler/v1alpha1/workflowrun.go
@@ -59,6 +65,45 @@ func CreateWorkflowRun(ctx context.Context, project, workflow, tenant string, wf
return handler.K8sClient.CycloneV1alpha1().WorkflowRuns(common.TenantNamespace(tenant)).Create(wfr)
}

func workflowRunCreationDryRun(project, workflow, tenant string, wfr *v1alpha1.WorkflowRun) (*v1alpha1.WorkflowRun, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This function is a little confused, which will make CreateWorkflowRun() more complex.

The target of this function is to check the disk free space before create the workflowrun.
I think it would be better to add an additional API for workflowrun precheck. We can add more precheck logic into this API later if necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dry-run could do the same thing with precheck API. I think there is no need to add another API as the dry-run is easier to understand.

iawia002
iawia002 reviewed Apr 15, 2020
View reviewed changes
release/cyclone-server.yaml
Comment on lines +67 to +68
request: 10Gi
limit: 10Gi
Copy link

Choose a reason for hiding this comment

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

LGTM

@supereagle
Copy link
Member

/lgtm

@caicloud-bot caicloud-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2020
@caicloud-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: supereagle

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

@caicloud-bot caicloud-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2020
@caicloud-bot caicloud-bot merged commit 49093a0 into caicloud:master Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supereagle supereagle supereagle left review comments

@iawia002 iawia002 iawia002 left review comments

Assignees

@supereagle supereagle

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zhujian7 @hyy0322 @supereagle @caicloud-bot @iawia002