-
Notifications
You must be signed in to change notification settings - Fork 72
feat: Runtime variables and system config change reload #352
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
Changes from 25 commits
64a078f
9c356df
9f6a1be
a983e26
d56b728
ba72b14
3074622
56777b4
504c4bf
c91efc7
b6c67dd
63011e1
7df9ac6
fddf2d6
2434864
72d6892
0e2eb15
8c8d4e2
8684e44
6fe23c7
efffe1f
aaf6d31
9e236aa
d823a95
e1ade14
ce9fd4b
49c1c6a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| -- +goose Up | ||
| ALTER TABLE workspaces DROP COLUMN url; | ||
|
|
||
| -- +goose Down | ||
| ALTER TABLE workspaces ADD COLUMN url TEXT; | ||
| UPDATE workspaces set url = ''; | ||
| ALTER TABLE workspaces ALTER COLUMN url SET NOT NULL; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,11 +6,11 @@ import ( | |
| "errors" | ||
| "fmt" | ||
| sq "github.com/Masterminds/squirrel" | ||
| "github.com/ghodss/yaml" | ||
| "github.com/onepanelio/core/pkg/util/label" | ||
| "github.com/onepanelio/core/pkg/util/pagination" | ||
| "github.com/onepanelio/core/pkg/util/ptr" | ||
| uid2 "github.com/onepanelio/core/pkg/util/uid" | ||
| "gopkg.in/yaml.v2" | ||
| "io" | ||
| "io/ioutil" | ||
| "k8s.io/apimachinery/pkg/watch" | ||
|
|
@@ -328,7 +328,9 @@ func (c *Client) ValidateWorkflowExecution(namespace string, manifest []byte) (e | |
| return | ||
| } | ||
|
|
||
| func (c *Client) CreateWorkflowExecution(namespace string, workflow *WorkflowExecution) (*WorkflowExecution, error) { | ||
| // CreateWorkflowExecution creates an argo workflow execution and related resources. | ||
| // Note that the workflow template is loaded from the database/k8s, so workflow.WorkflowTemplate.Manifest is not used. | ||
| func (c *Client) CreateWorkflowExecution(namespace string, workflow *WorkflowExecution, runtimeVars *RuntimeVars) (*WorkflowExecution, error) { | ||
| workflowTemplate, err := c.GetWorkflowTemplate(namespace, workflow.WorkflowTemplate.UID, workflow.WorkflowTemplate.Version) | ||
| if err != nil { | ||
| log.WithFields(log.Fields{ | ||
|
|
@@ -339,6 +341,31 @@ func (c *Client) CreateWorkflowExecution(namespace string, workflow *WorkflowExe | |
| return nil, util.NewUserError(codes.NotFound, "Error with getting workflow template.") | ||
| } | ||
|
|
||
| if runtimeVars != nil && workflowTemplate.ArgoWorkflowTemplate != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this section should be here. It's unique to Workspaces only. Other calls to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rushtehrani I agree, we need to separate the concerns here. The code above gets the workflow template and that also gets the k8s argo template. At the moment, we can't modify it beforehand because it gets it in this function. We could change the function so that we pass in the workflow template with the argo contents pre-loaded. What do you think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Vafilor parameters are showing and configuration reload is working. Is this the only change that remains?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rushtehrani Yes, I should have it ready by tomorrow morning. |
||
| argoTemplate := workflowTemplate.ArgoWorkflowTemplate | ||
| for _, param := range runtimeVars.AdditionalParameters { | ||
| argoTemplate.Spec.Arguments.Parameters = append(argoTemplate.Spec.Arguments.Parameters, wfv1.Parameter{ | ||
| Name: param.Name, | ||
| Value: param.Value, | ||
| }) | ||
| } | ||
|
|
||
| finalTemplates := make([]wfv1.Template, 0) | ||
| for i := range argoTemplate.Spec.Templates { | ||
| template := &argoTemplate.Spec.Templates[i] | ||
|
|
||
| if template.Name == "stateful-set-resource" { | ||
| template.Resource.Manifest = runtimeVars.StatefulSetManifest | ||
| } | ||
|
|
||
| if template.Name != runtimeVars.VirtualService.Name { | ||
| finalTemplates = append(finalTemplates, *template) | ||
| } | ||
| } | ||
| finalTemplates = append(finalTemplates, *runtimeVars.VirtualService) | ||
| workflowTemplate.ArgoWorkflowTemplate.Spec.Templates = finalTemplates | ||
| } | ||
|
|
||
| // TODO: Need to pull system parameters from k8s config/secret here, example: HOST | ||
| opts := &WorkflowExecutionOptions{ | ||
| Labels: &map[string]string{}, | ||
|
|
@@ -416,7 +443,7 @@ func (c *Client) CloneWorkflowExecution(namespace, uid string) (*WorkflowExecuti | |
| return nil, err | ||
| } | ||
|
|
||
| return c.CreateWorkflowExecution(namespace, workflowExecution) | ||
| return c.CreateWorkflowExecution(namespace, workflowExecution, nil) | ||
| } | ||
|
|
||
| func (c *Client) insertPreWorkflowExecutionStatistic(namespace, name string, workflowTemplateVersionId uint64, createdAt time.Time, uid string, parameters []Parameter) (newId uint64, err error) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,10 +98,21 @@ func injectWorkspaceSystemParameters(namespace string, workspace *Workspace, wor | |
| } | ||
|
|
||
| func (c *Client) createWorkspace(namespace string, parameters []byte, workspace *Workspace) (*Workspace, error) { | ||
| _, err := c.CreateWorkflowExecution(namespace, &WorkflowExecution{ | ||
| systemConfig, err := c.GetSystemConfig() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| runtimeVars, err := workspace.WorkspaceTemplate.RuntimeVars(systemConfig) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| _, err = c.CreateWorkflowExecution(namespace, &WorkflowExecution{ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Vafilor in reference to this comment. Ideally this we want to make the override before they're passed into |
||
| Parameters: workspace.Parameters, | ||
| WorkflowTemplate: workspace.WorkspaceTemplate.WorkflowTemplate, | ||
| }) | ||
| }, runtimeVars) | ||
|
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -116,7 +127,6 @@ func (c *Client) createWorkspace(namespace string, parameters []byte, workspace | |
| "started_at": time.Now().UTC(), | ||
| "workspace_template_id": workspace.WorkspaceTemplate.ID, | ||
| "workspace_template_version": workspace.WorkspaceTemplate.Version, | ||
| "url": workspace.URL, | ||
| }). | ||
| Suffix("RETURNING id, created_at"). | ||
| RunWith(c.DB). | ||
|
|
@@ -154,7 +164,6 @@ func (c *Client) CreateWorkspace(namespace string, workspace *Workspace) (*Works | |
| if sysHost == nil { | ||
| return nil, fmt.Errorf("sys-host parameter not found") | ||
| } | ||
| workspace.URL = *sysHost | ||
|
|
||
| existingWorkspace, err := c.GetWorkspace(namespace, workspace.UID) | ||
| if err != nil { | ||
|
|
@@ -394,7 +403,7 @@ func (c *Client) updateWorkspace(namespace, uid, workspaceAction, resourceAction | |
| _, err = c.CreateWorkflowExecution(namespace, &WorkflowExecution{ | ||
| Parameters: workspace.Parameters, | ||
| WorkflowTemplate: workspace.WorkspaceTemplate.WorkflowTemplate, | ||
| }) | ||
| }, nil) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
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.
Let's just call this
Domain. I think we should also haveFQDNhere too.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.
@rushtehrani Makes sense, and sure thing. I'm slowly adding them as I encounter them in the code.