-
Notifications
You must be signed in to change notification settings - Fork 1.5k
AGENT-1302,OCPBUGS-61668: Merge interactive ignition into unconfigured-ignition #9941
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 all commits
6ab31ee
5326b68
8d9dce9
00f6ab7
e3968cf
310297a
32fe754
b1ebd47
5c8393c
f8bb47e
9dc636f
5cdd482
bfcc946
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 |
|---|---|---|
| @@ -1,14 +1,13 @@ | ||
| [Unit] | ||
| Description=Extract agent-tui at boot | ||
| After=selinux.service start-local-registry.service | ||
| Before=agent-interactive-console.service | ||
| ConditionPathExists=/etc/assisted/rendezvous-host.env | ||
| ConditionPathExists=/etc/assisted/rendezvous-host.env.template | ||
| ConditionPathExists=/etc/assisted/interactive-ui | ||
| ConditionPathExists=!/usr/local/bin/agent-tui | ||
|
|
||
| [Service] | ||
| Type=oneshot | ||
| ExecStart=/usr/local/bin/agent-extract-tui.sh | ||
| TimeoutStartSec=300s | ||
|
|
||
| [Install] | ||
| WantedBy=getty@tty1.service | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,6 @@ | ||
| ### This is currently a draft placeholder for running the UI, to be completed | ||
|
|
||
| [Unit] | ||
| Description=Service that runs the Agent Installer UI | ||
| Description=Agent Installer Web UI | ||
| Wants=network-online.target assisted-service.service | ||
| Conflicts=agent-register-cluster.service agent-import-cluster.service | ||
|
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. Any specific reason to remove also
Member
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. Conflicts are evaluated when systemd builds the transaction, but Conditions are only evaluated at runtime. So Conflicts are dangerous to use in combination with Condtions, because they could cause neither service to run. The add-node ignition will not have
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. 🤔 do you maybe mean that we should also review also our other services where this combo is present, relying only on the
Member
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. Doesn't hurt to check, but I believe none of those conflicting services are ever enabled+Wanted at the same time. |
||
| After=network-online.target assisted-service.service | ||
| PartOf=assisted-service-pod.service | ||
| ConditionPathExists=/etc/assisted/node0 | ||
|
|
@@ -13,15 +10,15 @@ ConditionPathExists=/etc/assisted/interactive-ui | |
| Environment=PODMAN_SYSTEMD_UNIT=%n | ||
| EnvironmentFile=/usr/local/share/assisted-service/agent-images.env | ||
| EnvironmentFile=/etc/assisted/rendezvous-host.env | ||
| Restart=on-failure | ||
| ExecStartPre=/bin/rm -f %t/%n.ctr-id | ||
| ExecStartPre=/usr/local/bin/wait-for-assisted-service.sh | ||
| ExecStart=/usr/bin/podman run --net host --cidfile=%t/%n.ctr-id --cgroups=no-conmon --log-driver=journald --restart=on-failure:10 --pod-id-file=%t/assisted-service-pod.pod-id --replace -d --name=agent-installer-ui --env AIUI_APP_API_URL $INSTALLER_UI_IMAGE | ||
| ExecStart=/usr/bin/podman run --net host --cidfile=%t/%n.ctr-id --cgroups=no-conmon --log-driver=journald --rm --pod-id-file=%t/assisted-service-pod.pod-id --replace -d --name=agent-installer-ui --env AIUI_APP_API_URL $INSTALLER_UI_IMAGE | ||
| ExecStop=/usr/bin/podman stop --ignore --cidfile=%t/%n.ctr-id | ||
| ExecStopPost=/usr/bin/podman rm -f --ignore --cidfile=%t/%n.ctr-id | ||
|
|
||
| KillMode=mixed | ||
| Type=oneshot | ||
| RemainAfterExit=true | ||
| Type=notify | ||
|
|
||
| [Install] | ||
| WantedBy=multi-user.target | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,12 @@ | ||
| package image | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "html/template" | ||
| "net" | ||
| "net/url" | ||
| "path" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
@@ -306,7 +306,7 @@ func (a *Ignition) Generate(ctx context.Context, dependencies asset.Parents) err | |
|
|
||
| rendezvousHostFile := ignition.FileFromString(rendezvousHostEnvPath, | ||
| "root", 0644, | ||
| getRendezvousHostEnv(agentTemplateData.ServiceProtocol, a.RendezvousIP, authConfig.AgentAuthToken, authConfig.UserAuthToken, agentWorkflow.Workflow)) | ||
| getRendezvousHostEnv(agentTemplateData, a.RendezvousIP, agentWorkflow.Workflow)) | ||
| config.Storage.Files = append(config.Storage.Files, rendezvousHostFile) | ||
|
|
||
| err = addBootstrapScripts(&config, agentManifests.ClusterImageSet.Spec.ReleaseImage) | ||
|
|
@@ -370,11 +370,13 @@ func (a *Ignition) Generate(ctx context.Context, dependencies asset.Parents) err | |
|
|
||
| func getDefaultEnabledServices() []string { | ||
| return []string{ | ||
| "agent-extract-tui.service", | ||
| "agent-interactive-console.service", | ||
| "agent-interactive-console-serial@.service", | ||
| "agent-register-cluster.service", | ||
| "agent-import-cluster.service", | ||
| "agent-register-infraenv.service", | ||
| "agent-ui.service", | ||
| "agent.service", | ||
| "assisted-service-db.service", | ||
| "assisted-service-pod.service", | ||
|
|
@@ -446,17 +448,12 @@ func getTemplateData(name, pullSecret, releaseImageList, releaseImage, releaseIm | |
| } | ||
| } | ||
|
|
||
| func getRendezvousHostEnv(serviceProtocol, nodeZeroIP, agentAuthtoken, userAuthToken string, workflowType workflow.AgentWorkflowType) string { | ||
| serviceBaseURL := url.URL{ | ||
| Scheme: serviceProtocol, | ||
| Host: net.JoinHostPort(nodeZeroIP, "8090"), | ||
| Path: "/", | ||
| } | ||
| imageServiceBaseURL := url.URL{ | ||
| Scheme: serviceProtocol, | ||
| Host: net.JoinHostPort(nodeZeroIP, "8888"), | ||
| Path: "/", | ||
| } | ||
| func getRendezvousHostEnvTemplate(data *agentTemplateData, workflowType workflow.AgentWorkflowType) string { | ||
| host := "{{ if $isIPv6 }}{{ printf \"[%s]\" .RendezvousIP }}{{ else }}{{ .RendezvousIP }}{{ end }}" | ||
| serviceBaseURL := fmt.Sprintf("%s://%s:8090/", data.ServiceProtocol, host) | ||
| imageServiceBaseURL := fmt.Sprintf("%s://%s:8888/", data.ServiceProtocol, host) | ||
| uiBaseURL := fmt.Sprintf("%s://%s:3001/", data.ServiceProtocol, host) | ||
|
|
||
| // USER_AUTH_TOKEN is required to authenticate API requests against agent-installer-local auth type | ||
| // and for the endpoints marked with userAuth security definition in assisted-service swagger.yaml. | ||
| // PULL_SECRET_TOKEN contains the AGENT_AUTH_TOKEN and is required for the endpoints marked with agentAuth security definition in assisted-service swagger.yaml. | ||
|
|
@@ -469,27 +466,40 @@ func getRendezvousHostEnv(serviceProtocol, nodeZeroIP, agentAuthtoken, userAuthT | |
| // and ensure successful authentication. | ||
| // In the absence of PULL_SECRET_TOKEN, the cluster installation will wait forever. | ||
|
|
||
| rendezvousHostEnv := fmt.Sprintf(`NODE_ZERO_IP=%s | ||
| rendezvousHostEnvTemplate := fmt.Sprintf(`#{{ $isIPv6 := false }}{{ $host := .RendezvousIP }}{{ range ( len .RendezvousIP ) }}{{if eq ( index ( slice $host . ) 0 ) ':'}}{{ $isIPv6 = true }}{{ end }}{{ end }} | ||
|
zaneb marked this conversation as resolved.
|
||
| NODE_ZERO_IP={{.RendezvousIP}} | ||
| SERVICE_BASE_URL=%s | ||
| IMAGE_SERVICE_BASE_URL=%s | ||
| PULL_SECRET_TOKEN=%s | ||
| USER_AUTH_TOKEN=%s | ||
| WORKFLOW_TYPE=%s | ||
| `, nodeZeroIP, serviceBaseURL.String(), imageServiceBaseURL.String(), agentAuthtoken, userAuthToken, workflowType) | ||
|
|
||
| if workflowType == workflow.AgentWorkflowTypeInstallInteractiveDisconnected { | ||
| uiBaseURL := url.URL{ | ||
| Scheme: serviceProtocol, | ||
| Host: net.JoinHostPort(nodeZeroIP, "3001"), | ||
| Path: "/", | ||
| } | ||
| uiEnv := fmt.Sprintf(`AIUI_APP_API_URL=%s | ||
| AIUI_APP_API_URL=%s | ||
| AIUI_URL=%s | ||
| `, serviceBaseURL.String(), uiBaseURL.String()) | ||
| rendezvousHostEnv = fmt.Sprintf("%s%s", rendezvousHostEnv, uiEnv) | ||
| `, serviceBaseURL, imageServiceBaseURL, data.AgentAuthToken, data.UserAuthToken, workflowType, serviceBaseURL, uiBaseURL) | ||
|
|
||
| return rendezvousHostEnvTemplate | ||
| } | ||
|
|
||
| func getRendezvousHostEnvFromTemplate(hostEnvTemplate, nodeZeroIP string) (string, error) { | ||
| tmpl, err := template.New("rendezvous-host.env").Parse(hostEnvTemplate) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| buf := &bytes.Buffer{} | ||
| if err := tmpl.Execute(buf, struct{ RendezvousIP string }{nodeZeroIP}); err != nil { | ||
| return "", err | ||
| } | ||
| return buf.String(), nil | ||
| } | ||
|
|
||
| return rendezvousHostEnv | ||
| func getRendezvousHostEnv(data *agentTemplateData, nodeZeroIP string, workflowType workflow.AgentWorkflowType) string { | ||
| env, err := getRendezvousHostEnvFromTemplate( | ||
| getRendezvousHostEnvTemplate(data, workflowType), | ||
| nodeZeroIP) | ||
| if err != nil { | ||
| panic(err) | ||
|
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 was expecting to return an error here rather than a panic
Member
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. The template that we are resolving is hard-coded here in the source. If that can't be resolved then it isn't due to bad input, just straight up bad code. We should fail as early and loudly as possible. The function signature hasn't changed; it never returned an error. |
||
| } | ||
| return env | ||
| } | ||
|
|
||
| func getAddNodesEnv(clusterInfo joiner.ClusterInfo, authTokenExpiry string) string { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ package image | |
|
|
||
| import ( | ||
| "context" | ||
| "net/url" | ||
| "fmt" | ||
| "os" | ||
| "path" | ||
| "path/filepath" | ||
|
|
@@ -151,6 +151,7 @@ func (a *UnconfiguredIgnition) Generate(_ context.Context, dependencies asset.Pa | |
| a.CPUArch = *osImage.CPUArchitecture | ||
|
|
||
| agentTemplateData := &agentTemplateData{ | ||
| ServiceProtocol: "http", | ||
| PullSecret: pullSecretAsset.GetPullSecretData(), | ||
| ReleaseImages: releaseImageList, | ||
| ReleaseImage: clusterImageSet.Spec.ReleaseImage, | ||
|
|
@@ -160,46 +161,45 @@ func (a *UnconfiguredIgnition) Generate(_ context.Context, dependencies asset.Pa | |
| InfraEnvID: infraEnvID, | ||
| OSImage: osImage, | ||
| Proxy: infraEnv.Spec.Proxy, | ||
| AuthType: "none", | ||
| } | ||
|
|
||
| enabledServices := getDefaultEnabledServices() | ||
|
|
||
| rendezvousHostTemplateData := getRendezvousHostEnvTemplate(agentTemplateData, agentWorkflow.Workflow) | ||
| rendezvousHostTemplateFile := ignition.FileFromString(fmt.Sprintf("%s.template", rendezvousHostEnvPath), "root", 0644, rendezvousHostTemplateData) | ||
| config.Storage.Files = append(config.Storage.Files, rendezvousHostTemplateFile) | ||
|
|
||
| rendezvousIP, err := RetrieveRendezvousIP(agentConfig.Config, nil, nmStateConfigs.Config) | ||
| if err == nil { | ||
| rendezvousHostData, err := getRendezvousHostEnvFromTemplate(rendezvousHostTemplateData, rendezvousIP) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| rendezvousHostFile := ignition.FileFromString(rendezvousHostEnvPath, "root", 0644, rendezvousHostData) | ||
|
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. So there could be the case where both the
Member
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. Keeping the template in the
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. Couldn't be immediately removed? AFAIK the rendezvousIP is always empty in the disconnected workflow (until now - the only expected integration will arrive from the SaaS UI that will generate the agent-config.yaml, something now captured)
Member
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. Since openshift/agent-installer-utils#156 landed, it probably could be removed. At the time I didn't want to create an ordering dependency with that PR, because that's a sure way to not get things landed, and right now I don't want to delay landing this in order to test any changes to the existing |
||
| config.Storage.Files = append(config.Storage.Files, rendezvousHostFile) | ||
| } | ||
|
|
||
| switch agentWorkflow.Workflow { | ||
| case workflow.AgentWorkflowTypeInstall: | ||
| agentTemplateData.ConfigImageFiles = strings.Join(GetConfigImageFiles(), ",") | ||
|
|
||
| // Enable the agent-check-config-image.service for the current workflow. | ||
| enabledServices = append(enabledServices, "agent-check-config-image.service") | ||
|
|
||
| case workflow.AgentWorkflowTypeInstallInteractiveDisconnected: | ||
| // Add the rendezvous host file. Agent TUI will interact with that file in case | ||
| // the rendezvous IP wasn't previously configured, by managing it as a template file. | ||
| rendezvousIP := "{{.RendezvousIP}}" | ||
| if agentConfig.Config != nil { | ||
| rendezvousIP = agentConfig.Config.RendezvousIP | ||
| } | ||
| // Avoids escaping in case the template parameter was used. | ||
| rendezvousHostData, err := url.QueryUnescape(getRendezvousHostEnv("http", rendezvousIP, "", "", agentWorkflow.Workflow)) | ||
| if err != nil { | ||
| return err | ||
| if rendezvousIP == "" { | ||
| rendezvousHostFile := ignition.FileFromString(rendezvousHostEnvPath, "root", 0644, rendezvousHostTemplateData) | ||
| config.Storage.Files = append(config.Storage.Files, rendezvousHostFile) | ||
| } | ||
| rendezvousHostFile := ignition.FileFromString(rendezvousHostEnvPath, "root", 0644, rendezvousHostData) | ||
| config.Storage.Files = append(config.Storage.Files, rendezvousHostFile) | ||
|
|
||
| // Explicitly disable the load-config-iso service, not required in the current flow | ||
| // (even though disabled by default, the udev rule may require it). | ||
| config.Storage.Files = append(config.Storage.Files, ignition.FileFromString("/etc/assisted/no-config-image", "root", 0644, "")) | ||
|
|
||
| // Enable the UI service. | ||
| enabledServices = append(enabledServices, "agent-start-ui.service") | ||
| interactiveUIFile := ignition.FileFromString("/etc/assisted/interactive-ui", "root", 0644, "") | ||
| config.Storage.Files = append(config.Storage.Files, interactiveUIFile) | ||
|
|
||
| // Enable the agent-extract-tui service | ||
| enabledServices = append(enabledServices, "agent-extract-tui.service") | ||
|
|
||
| // Let's disable the assisted-service authentication. | ||
| agentTemplateData.AuthType = "none" | ||
| } | ||
|
|
||
| // Required by assisted-service. | ||
|
|
@@ -242,6 +242,8 @@ func (a *UnconfiguredIgnition) Generate(_ context.Context, dependencies asset.Pa | |
| config.Storage.Files = append(config.Storage.Files, manifestFile) | ||
| } | ||
|
|
||
| // the agent-check-config-image.service added only to the unconfigured ignition | ||
| enabledServices = append(enabledServices, "agent-check-config-image.service") | ||
| err = bootstrap.AddSystemdUnits(&config, "agent/systemd/units", agentTemplateData, enabledServices) | ||
| if err != nil { | ||
| return err | ||
|
|
||
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.
IIUC, this condition is required to allow enabling by the default the
agent-extract-tui.service, so that it will not conflict during the regular workflow (which takes care of extracting the agent-tui artifacts from the initrd) - with the final intention of simplifying the unconfigured-ignition asset? If so a small comment could be useful hereThere 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.
TBH it seemed self-explanatory to me that if we already have the agent-tui binary then we don't need to run a service to get it?
It already wouldn't happen in the regular workflow because we have always had
ConditionPathExists=/etc/assisted/interactive-ui. So this condition isn't required at all. But one day when we enable the UI in the regular agent install, it will be one less bug that somebody has to fix.This is described in the commit message for anybody who needs to know the context in future.
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.
At least for me it was not immediately evident, since the service was expected to run only once (and currently only when the UI is present). And btw, I think in future we should change the triggering condition for this service, detaching completely from the UI and binding it to a more explicit one (ie something like
/etc/assisted/no-registry) indicating that the payload is immediately available for consumption (and the same could be used by the TUI to skip the release check).Yeah the commit message was useful (previous link seems broken now, I think it's this one), even though also a comment wouldn't have hurt IMHO
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.
Right now the appliance has the registry, but I believe does not have the agent-tui, and we don't want it to use the agent-tui because we don't want it creating the rendezvous-host.env file - only the config-iso should do that. One day we might want the GUI to be available in the appliance, but by conditioning on interactive-ui we are punting until then.