Skip to content

[OCTRL-1018] Once NewEnvironmentAsync returns, the env should exist #710

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

Closed
justonedev1 wants to merge 1 commit into from
Closed

[OCTRL-1018] Once NewEnvironmentAsync returns, the env should exist #710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions core/environment/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func (envs *Manager) GetActiveDetectors() system.IDMap {
return response
}

func (envs *Manager) CreateEnvironment(workflowPath string, userVars map[string]string, public bool, newId uid.ID, autoTransition bool) (uid.ID, error) {
func (envs *Manager) CreateEnvironment(workflowPath string, userVars map[string]string, public bool, newId uid.ID) (uid.ID, error) {
// Before we load the workflow, we get the list of currently active detectors. This query must be performed before
// loading the workflow in order to compare the currently used detectors with the detectors required by the newly
// created environment.
Expand Down Expand Up @@ -403,7 +403,19 @@ func (envs *Manager) CreateEnvironment(workflowPath string, userVars map[string]
WithField("level", infologger.IL_Devel).
Debug("envman write unlock")

err = env.TryTransition(NewDeployTransition(
return env.id, nil
}

func (envs *Manager) RunEnvironment(workflowPath string, envId uid.ID, autoTransition bool) error {
envs.mu.Lock()
env, ok := envs.m[envId]
envs.mu.Unlock()

if !ok {
return errors.New(fmt.Sprintf("trying to run unknown env id: %v", envId))
}

err := env.TryTransition(NewDeployTransition(
envs.taskman,
nil, // roles,
nil),
Expand Down Expand Up @@ -524,7 +536,7 @@ func (envs *Manager) CreateEnvironment(workflowPath string, userVars map[string]
}()
}

return env.id, err
return err
}

// Deployment/configuration failure code path starts here
Expand Down Expand Up @@ -567,7 +579,7 @@ func (envs *Manager) CreateEnvironment(workflowPath string, userVars map[string]
Info("environment deployment failed, tasks were cleaned up")
log.WithField("partition", env.Id().String()).Info("environment teardown complete")

return env.id, err
return err
}

func (envs *Manager) TeardownEnvironment(environmentId uid.ID, force bool) error {
Expand Down
41 changes: 37 additions & 4 deletions core/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,13 +309,13 @@ func (m *RpcServer) doNewEnvironmentAsync(cxt context.Context, userVars map[stri
// we store the last known request user in the environment
lastRequestUserJ, _ := json.Marshal(requestUser)
userVars["last_request_user"] = string(lastRequestUserJ[:])
id, err = m.state.environments.CreateEnvironment(workflowTemplate, userVars, public, id, autoTransition)
err = m.state.environments.RunEnvironment(workflowTemplate, id, autoTransition)
if err != nil {
the.EventWriterWithTopic(topic.Environment).WriteEvent(&evpb.Ev_EnvironmentEvent{
EnvironmentId: id.String(),
State: "ERROR",
Error: err.Error(),
Message: "cannot create new environment", // GUI listens for this concrete string
Message: "cannot run new environment", // GUI listens for this concrete string
LastRequestUser: requestUser,
WorkflowTemplateInfo: &evpb.WorkflowTemplateInfo{
Public: public,
Expand Down Expand Up @@ -374,7 +374,26 @@ func (m *RpcServer) NewEnvironmentAsync(cxt context.Context, request *pb.NewEnvi
}
defer setCurrentUnixMilli(&reply.Timestamp)

go m.doNewEnvironmentAsync(cxt, userVars, request.GetRequestUser(), request.GetWorkflowTemplate(), request.GetPublic(), request.GetAutoTransition(), id)
public := request.GetPublic()
workflowTemplate := request.GetWorkflowTemplate()
requestUser := request.GetRequestUser()

id, err = m.state.environments.CreateEnvironment(request.GetWorkflowTemplate(), userVars, request.GetPublic(), id)
Copy link
Collaborator

@knopers8 knopers8 May 20, 2025

Choose a reason for hiding this comment

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

One has to recognize that this will move environment creation into the synchronous part, which is a change in the behaviour. I imagine that the creation of an environment with all FLPs and no cached workflow templates may take 1-2 minutes. Please make sure that this is OK with George.

Copy link
Collaborator Author

@justonedev1 justonedev1 May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I know, but this is the only way how to achieve George's desired behavior without changing order of operations and other code changes.

And it is also the reason why I gave binary with these changes to STG for him to play with so he can see how it will behave.

I also don't want to merge this before he okays this

if err != nil {
the.EventWriterWithTopic(topic.Environment).WriteEvent(&evpb.Ev_EnvironmentEvent{
EnvironmentId: id.String(),
State: "ERROR",
Error: err.Error(),
Message: "cannot create new environment", // GUI listens for this concrete string
LastRequestUser: requestUser,
WorkflowTemplateInfo: &evpb.WorkflowTemplateInfo{
Public: public,
Path: workflowTemplate,
},
})
return
}
go m.doNewEnvironmentAsync(cxt, userVars, requestUser, workflowTemplate, public, request.GetAutoTransition(), id)

return
}
Expand Down Expand Up @@ -421,7 +440,7 @@ func (m *RpcServer) NewEnvironment(cxt context.Context, request *pb.NewEnvironme

// Create new Environment instance with some roles, we get back a UUID
id := uid.New()
id, err = m.state.environments.CreateEnvironment(request.GetWorkflowTemplate(), inputUserVars, request.GetPublic(), id, request.GetAutoTransition())
id, err = m.state.environments.CreateEnvironment(request.GetWorkflowTemplate(), inputUserVars, request.GetPublic(), id)
if err != nil {
st := status.Newf(codes.Internal, "cannot create new environment: %s", utils.TruncateString(err.Error(), MAX_ERROR_LENGTH))
ei := &pb.EnvironmentInfo{
Expand All @@ -436,6 +455,20 @@ func (m *RpcServer) NewEnvironment(cxt context.Context, request *pb.NewEnvironme
return
}

err = m.state.environments.RunEnvironment(request.GetWorkflowTemplate(), id, request.GetAutoTransition())
if err != nil {
st := status.Newf(codes.Internal, "cannot run new environment: %s", utils.TruncateString(err.Error(), MAX_ERROR_LENGTH))
ei := &pb.EnvironmentInfo{
Id: id.String(),
CreatedWhen: time.Now().UnixMilli(),
State: "ERROR", // not really, but close
NumberOfFlps: 0,
Copy link
Collaborator

@knopers8 knopers8 May 20, 2025

Choose a reason for hiding this comment

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

would it be difficult to have a correct value for the NumberOfFlps?

Copy link
Collaborator Author

@justonedev1 justonedev1 May 20, 2025

Choose a reason for hiding this comment

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

no idea, I didn't change anything, I just split the code.

}
st, _ = st.WithDetails(ei)
err = st.Err()

return
}
newEnv, err := m.state.environments.Environment(id)
if err != nil {
st := status.Newf(codes.Internal, "cannot get newly created environment: %s", utils.TruncateString(err.Error(), MAX_ERROR_LENGTH))
Expand Down