deployment status #262

RaviKhandavilli · 2022-11-07T18:18:57Z

1.Code added to forward deployment status to controller .
2.Changed deployer logic to deploy each task separately so as to update task status of individual task.
3.Changed release name of the agents to contain job id and task id (only first 12 characters of task id ) in the place of job id and compute id.
4.Made some refactoring changes to use global constants instead of local strings.
Note :- Deployment status only represents whether the job added is deployed/failed/revoked. it does not contain task status.task status is directly communicated back to controller by flamelet agents.
DB changes :-
1.created a separate collection deployment to mongodb to specifically maintain deployment status of computes.

myungjin

left comments. please consider to address them.

myungjin · 2022-11-07T22:05:53Z

pkg/openapi/controller/api_computes_service.go

-	return openapi.Response(http.StatusNotImplemented, nil), errors.New("PutDeploymentStatus method not implemented")
+func (s *ComputesApiService) PutDeploymentStatus(ctx context.Context, computeId string, jobId string, xAPIKEY string,
+	deploymentStatus openapi.DeploymentStatus) (openapi.ImplResponse, error) {
+	err := s.dbService.UpdateDeploymentStatus(deploymentStatus)


Two comments are here:

This code doesn't use computeId passed as argument. It blindly uses computeId in deploymentStatus. This can potentially pollute the data in the DB at a minimum and be exploited for security vulnerability.

This is a restapi call; so, one can create any arbitrary message without going through flamectl.

Also, please reconsider whether the computeId field in the message is needed since it is fed as an argument.

Changed api endpoint to send only agent statuses obj , instead of deployment statuses obj.
Constructing deployment status obj locally from both params(job id and compute id) and body.

myungjin · 2022-11-07T22:12:18Z

pkg/openapi/controller/api_computes_service.go

+	deploymentStatus openapi.DeploymentStatus) (openapi.ImplResponse, error) {
+	err := s.dbService.UpdateDeploymentStatus(deploymentStatus)
+	if err != nil {
+		errMsg := fmt.Errorf("failed to update deployent status for computeid %s, jobid %s : %v", err, computeId, jobId)


Suggested change

errMsg := fmt.Errorf("failed to update deployent status for computeid %s, jobid %s : %v", err, computeId, jobId)

errMsg := fmt.Errorf("failed to update deployment status for computeId %s, jobId %s : %v", computeId, jobId, err)

cmd/controller/app/database/mongodb/deployment.go

myungjin · 2022-11-07T22:17:28Z

api/computes_components.partials.yml

@@ -101,10 +101,10 @@ DeploymentStatus:
  properties:
    jobId:
      type: string
+    computeId:


Can you explain why computeId is needed? As the url path that uses DeploymentStatus has computeId in it, do we really need to duplicate computeId here?

we need this encapsulation of job id and compute id to make inserting data to MongoDB simple, also will be useful while implementing get multiple deployment status codes.
Anyways for put call I have changed message to contain only agent-statuses obj instead of deployment status obj.

myungjin · 2022-11-08T01:06:15Z

cmd/controller/app/database/mongodb/deployment.go

+	if result.Err() == nil {
+		taskStatus := map[string]openapi.AgentState{}
+		for key, val := range deploymentStatus.AgentStatuses {
+			taskStatus["agentstatuses."+key] = val


can you make agentstatuses as a constant?

myungjin · 2022-11-08T01:35:19Z

cmd/deployer/app/resource_handler.go

 	// 2.delete all the job resource specification files
 	deploymentChartPath := filepath.Join(deploymentDirPath, jobId)
 	_ = os.RemoveAll(deploymentChartPath)
+	if err != nil {


err is a variable defined from getDeploymentConfig call. So, something doesn't seem right.

myungjin · 2022-11-08T01:37:03Z

cmd/deployer/app/resource_handler.go

 		}
 	}
-
 	// 2.delete all the job resource specification files
 	deploymentChartPath := filepath.Join(deploymentDirPath, jobId)


There is no such directory that only ends with jobId because now the helm-chart directory consists of three elements: deploymentDirPath, jobId and taskId (see https://github.com/cisco-open/flame/pull/262/files#diff-9d9bde33b386c8e731e98f3cdc370505e45708cf8274a6b701ea7e58b6858353R312).

Each directory should be deleted separately only if revoke is successful. Therefore lines 248-249 should be moved into for taskId := range cfg.AgentKVs { ... } loop.

myungjin · 2022-11-08T01:42:54Z

cmd/deployer/app/resource_handler.go

+	deploymentStatus := openapi.DeploymentStatus{
+		JobId:         jobId,
+		ComputeId:     r.spec.ComputeId,
+		AgentStatuses: map[string]openapi.AgentState{taskId: status},


Please modify this function so that it can create a request for many agents instead of one agent.

Modified this function to create request for many agents.

myungjin · 2022-11-08T01:46:03Z

cmd/deployer/app/resource_handler.go

+		err = r.dplyr.Install("job-"+deploymentConfig.JobId+"-"+taskId[:12], deploymentChartPath)
+		if err != nil {
+			errMsg := fmt.Sprintf("failed to deploy tasks: %v", err)
+			r.postDeploymentStatus(deploymentConfig.JobId, taskId, openapi.AGENT_FAILED)


Can you also do this in a batch fashion so that the number requests can reduced?

changed it to update statuses in batch

myungjin · 2022-11-08T01:46:10Z

cmd/deployer/app/resource_handler.go

+			r.postDeploymentStatus(deploymentConfig.JobId, taskId, openapi.AGENT_FAILED)
+			return fmt.Errorf(errMsg)
+		}
+		r.postDeploymentStatus(deploymentConfig.JobId, taskId, openapi.AGENT_DEPLOYED)


myungjin

left a few minor comments; otherwise, it looks good to me. please consider to address them. this pr will be ready in the next revision.

myungjin · 2022-11-08T17:04:36Z

api/computes_api.partials.yml

@@ -322,7 +322,7 @@
      content:
          application/json:
            schema:
-              $ref: '#/components/schemas/DeploymentStatus'
+              $ref: '#/components/schemas/AgentStatus'


In the api spec, there are two components: AgentStatus and DeploymentStatus.

I think we should use only one. Currently this change creates inconsistency (see https://github.com/cisco-open/flame/pull/262/files#diff-7371b2cf2240f3412cb798b7b95ab71759c3c637e0dceaf7492ff745b092b07cR376).

My recommendation is to keep DeploymentStatus as is but rename AgentStatus as DeploymentStatus. But remove the original DeploymentStatus in the components.

myungjin · 2022-11-09T18:11:27Z

cmd/deployer/app/resource_handler.go

+			uninstallErr := r.dplyr.Uninstall("job-" + jobId + "-" + taskId[:k8sShortLabelLength])
+			if uninstallErr != nil {
+				zap.S().Warnf("failed to release resources for job %s: %v", jobId, uninstallErr)
+				err = fmt.Errorf("%v: %v", err, uninstallErr)


This code snippet concatenates errors. The colon(:) separator makes it hard to separate out each errors when it is read by human engineers because colons can be used in an error. See an example from your PR: https://github.com/cisco-open/flame/pull/262/files#diff-9d9bde33b386c8e731e98f3cdc370505e45708cf8274a6b701ea7e58b6858353R332-R333.

So, it may be better to use some other symbol such as semi-colon.

Please take care of the similar issue in other places of this PR too.

myungjin · 2022-11-09T18:14:28Z

cmd/controller/app/database/mongodb/compute.go

@@ -173,3 +173,32 @@ func (db *MongoService) GetComputeById(computeId string) (openapi.ComputeSpec, e
 	}
 	return currentDocument, nil
 }
+
+func (db *MongoService) UpdateDeploymentStatus(computeId string, jobId string, agentStatuses map[string]openapi.AgentState) error {


Does this handle stale record issue correctly? If not, do you want to make it as a separate PR? If so, can you create a TODO comment in this function?

This overwrites the existing records , we are making sure to send status of all the tasks every time we put the deployment status.
instead of deleting existing records and creating new records we are sending status of all the new records in one batch. here we are simply rewriting existing record with the new data.

myungjin

lgtm

myungjin

please update commit comments.I am only seeing Co-authored-by: rakhanda <rakhanda@rakhandas-MBP.attlocal.net>.

Deployer forwards deployment status of each agents to controller. It deployes/revokes each agent one after one and pushes the status of operation.

myungjin

lgtm

Deployer forwards deployment status of each agents to controller. It deployes/revokes each agent one after one and pushes the status of operation.

RaviKhandavilli requested review from Divlik and myungjin November 7, 2022 18:29

RaviKhandavilli force-pushed the deployer-status branch from e5899a8 to 903cf62 Compare November 7, 2022 18:40

myungjin reviewed Nov 8, 2022

View reviewed changes

RaviKhandavilli requested a review from myungjin 6DAF November 8, 2022 16:53

myungjin reviewed Nov 9, 2022

View reviewed changes

RaviKhandavilli force-pushed the deployer-status branch from c50ab74 to 8e65907 Compare November 10, 2022 15:06

RaviKhandavilli requested a review from myungjin November 10, 2022 15:07

myungjin approved these changes Nov 10, 2022

View reviewed changes

myungjin requested changes Nov 10, 2022

View reviewed changes

deployment status

444926f

Deployer forwards deployment status of each agents to controller. It deployes/revokes each agent one after one and pushes the status of operation.

RaviKhandavilli force-pushed the deployer-status branch from 8e65907 to 444926f Compare November 10, 2022 18:22

RaviKhandavilli requested a review from myungjin November 10, 2022 18:23

myungjin approved these changes Nov 10, 2022

View reviewed changes

myungjin merged commit 873d61a into cisco-open:main Nov 10, 2022

myungjin linked an issue Nov 21, 2022 that may be closed by this pull request

[Feature] Propagate deployer agent creation errors to control plane #217

Closed

myungjin mentioned this pull request Nov 21, 2022

[Feature] Propagate deployer agent creation errors to control plane #217

Closed

dhruvsgarg pushed a commit to dhruvsgarg/flame that referenced this pull request Oct 18, 2024

deployment status (cisco-open#262)

c6eda09

Deployer forwards deployment status of each agents to controller. It deployes/revokes each agent one after one and pushes the status of operation.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

deployment status #262

deployment status #262

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	errMsg := fmt.Errorf("failed to update deployent status for computeid %s, jobid %s : %v", err, computeId, jobId)
	errMsg := fmt.Errorf("failed to update deployment status for computeId %s, jobId %s : %v", computeId, jobId, err)

deployment status #262

deployment status #262

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!