Make `/usr/bin/coreos-assembler` a Go program, implement `clean` in Go #2919

cgwalters · 2022-06-14T16:18:46Z

mantle: Display stderr from ssh-keygen

This is failing in CI, but on general principle we don't want
to swallow stderr.

Make /usr/bin/coreos-assembler a Go program, implement clean in Go

Converts the entrypoint into Go code
Add an internal library that exposes/wraps cmdlib.sh
because we have a lot of stuff in there that can't be ported
to Go yet.
Add an internal library for running inline (named) bash scripts
Port clean to Go

This is a pattern I think we'll use to aid the transition; rather
than trying to rewrite things wholesale in Go, we'll continue
to exec some shell scripts.

Gradually perhaps, we may invert some things and change both
cmdlib.sh and cmdlib.py to exec the cosa Go process in some
cases too.

Closes: #2821

cgwalters · 2022-06-14T16:53:39Z

To restate, the high level goal here is:

It becomes possible to use Go (alongside shell and Python) for "cosa"
It becomes discouraged/deprecated to write new cosa code in shell or Python

There's a ton of clear next steps here; the biggest is merging mantle/ to the toplevel, so that e.g. kola -> cosa; i.e. /usr/bin/cosa is a Go program containing the kola Go code, and we dispatch on argv[0] etc. We could do the same with gangplank/.

bgilbert · 2022-06-14T16:56:54Z

It becomes discouraged/deprecated to write new cosa code in shell or Python

I don't think there's consensus for this part. Restructuring for convenience is one thing; setting policy is another. Let's not block the former on the latter?

cgwalters · 2022-06-14T17:00:02Z

I don't think there's consensus for this part. Restructuring for convenience is one thing; setting policy is another. Let's not block the former on the latter?

Definitely agreed! I'm just setting out my goals/perspective.

miabbott · 2022-06-14T20:14:56Z

Would this be something that would be better suited for a dedicated branch?

Maybe it is better to keep this silo'ed from main until it is more fully featured? Just asking for other opinions.

cgwalters · 2022-06-14T22:47:13Z

Would this be something that would be better suited for a dedicated branch? Maybe it is better to keep this silo'ed from main until it is more fully featured?

Hmm...this PR is really small I think and unlikely to break anything - CI covers almost everything. The corner case I wrestled with turned out to be cosa shell where we need to carefully pass through stdin/stdout/stderr instead of buffering; which we do right now for everything but in the future won't.

Now if you're talking about the followups to this PR...I'm personally not planning on doing any wholesale or risky further followup changes in the short term. It could make sense though to branch if anyone chose to do that kind of thing.

I think it makes the most sense to land incremental changes to main though as much as we can; that's really what this is.

miabbott · 2022-06-16T14:01:31Z

I think it makes the most sense to land incremental changes to main though as much as we can; that's really what this is.

Thanks for sharing your perspective, Colin!

travier · 2022-06-17T15:55:43Z

I'm +1 for this but maybe we should have at least one command making use of it as an example?

jlebon

I'm also 👍 to this.

While I strongly agree we shouldn't start trying to rewrite everything in Go, I'd say at least the bash entrypoint itself would be worthwhile to rewrite to make this less confusing (and it's not large anyway).

Makefile

cgwalters · 2022-07-06T14:51:11Z

OK rebased 🏄
and this now depends on

cgwalters · 2022-07-06T14:52:20Z

While I strongly agree we shouldn't start trying to rewrite everything in Go, I'd say at least the bash entrypoint itself would be worthwhile to rewrite to make this less confusing (and it's not large anyway).

Also, this is done now!

cgwalters · 2022-07-06T19:05:51Z

[2022-07-06T18:53:13.389Z] FAIL: pxe-install (bios + metal) (0s)
[2022-07-06T18:53:13.389Z]     running ssh-keygen: exit status 255
[2022-07-06T18:53:13.389Z] Error: running ssh-keygen: exit status 255

Huh, fascinating 🕵️

cgwalters · 2022-07-07T15:51:52Z

OK so it took me a while to realize that this bit of bash code was...buggy:

#!/usr/bin/env bash
set -euo pipefail
...
# docker/podman don't run through PAM, but we want this set for the privileged
# (non-virtualized) path
export USER="${USER:-$(id -nu)}"

The thing is apparently because $( is run in a subshell or something, bash will ignore errors there. I think this whole time we've ended up with an empty USER environment variable in the OCP dynamic user case.

I adjusted the new Go logic to have a cleaner fallback here.

cgwalters · 2022-07-07T15:52:21Z

(also, this latest push faithfully ports the existing shell logic for /sys/fs/selinux to Go)

- Converts the entrypoint into Go code - Add an internal library that exposes/wraps `cmdlib.sh` because we have a lot of stuff in there that can't be ported to Go yet. - Add an internal library for running inline (named) bash scripts - Port `clean` to Go This is a pattern I think we'll use to aid the transition; rather than trying to rewrite things wholesale in Go, we'll continue to exec some shell scripts. Gradually perhaps, we may invert some things and change both `cmdlib.sh` and `cmdlib.py` to exec the cosa Go process in some cases too. Closes: coreos#2821

cgwalters · 2022-07-11T20:43:41Z

Nice, of course it randomly failed with a seemingly unrelated dynamic Python traceback...
Not reproducing...oh. Oh wait, I think this is fallout from recent changes to the CI config for openshift/os. We're just doing cosa build ostree and not cosa build. And of course that results in a dynamic Python crash when you cosa buildextend-aliyun.

This should fix coreos/coreos-assembler#2919 (comment) Fixes: c6e7dd9

cgwalters · 2022-07-11T20:55:04Z

CI failure is unrelated, I think openshift/os#889 will fix it

This should fix coreos/coreos-assembler#2919 (comment) Fixes: c6e7dd9 Fixes: coreos/coreos-assembler#2973

cgwalters · 2022-07-12T14:24:27Z

Hmm, timeout
/retest

cgwalters · 2022-07-12T17:37:20Z

@travier @mike-nguyen as TLs mind weighing in officially on this? 👍 / 👎 / 0 / other?

jlebon

Didn't do an in-depth review, but high-level LGTM! Thanks for tackling the entrypoint and cosa clean to get the ball rolling.

jlebon · 2022-07-12T18:40:32Z

internal/pkg/bashexec/bashexec.go

+		return nil, err
+	}
+
+	bashCmd := fmt.Sprintf("%s\n. /proc/self/fd/3\n", StrictMode)


Minor: strict mode can be passed in on the cmdline directly, e.g. bash -euo pipefail -c ....

jlebon · 2022-07-12T18:49:12Z

internal/pkg/cosash/cosash.go

+// Package cosash implements a "co-processing" proxy that is primarily
+// designed to expose a Go API that is currently implemented by `src/cmdlib.sh`.
+// A lot of the code in that file is stateful - e.g. APIs set environment variables
+// and allocate temporary directories.  So it wouldn't work very well to fork
+// a new shell process each time.
+//
+// The "co-processing" here is a way to describe that there's intended to be
+// a one-to-one relationship of the child bash process and the current one,
+// although this is not strictly required.  The Go APIs here call dynamically
+// into the bash process by writing to its stdin, and can receive serialized
+// data back over a pipe on file descriptor 3.


This is pretty cool! It borders on the "too magic" line, but I definitely see the appeal for our purposes here. I think one risk factor is the increased mental complexity of reading, writing, and debugging code leveraging this. Whereas if you have a whole cosa command you know is 100% Go, that's more straightforward to work with. That said, I think it's still worth trying out as we evaluate how best to clean up this codebase.

jlebon · 2022-07-12T19:24:16Z

I noticed this is marked as closing #2821 but I'm not sure we should tie the two. That cosa issue seems more to be a project policy decision.

cgwalters · 2022-07-12T20:04:56Z

I noticed this is marked as closing #2821 but I'm not sure we should tie the two. That cosa issue seems more to be a project policy decision.

There's a lot of fuzzy lines here...the title of that issue is just "default to Go" which one could interpret in multiple ways.

To me, "default to Go" is literally that the first code executed by /usr/bin/coreos-assembler is Go - which this PR does.

We did have discussions about the "policy" question of e.g. trying to actively discourage other languages which I think is still separate from that issue. (But I admit it can be read in that way)

dustymabe

Let's get the CI failure cleaned up and this merged.

cgwalters · 2022-07-13T14:25:16Z

CI is probably fixed by openshift/os#891
/retest

cgwalters · 2022-07-13T16:58:59Z

/retest

travier · 2022-07-25T12:33:13Z

@travier @mike-nguyen as TLs mind weighing in officially on this? +1 / -1 / 0 / other?

I'm late here but I've already agreed in principle in #2919 (comment) and in general I'm in favor of us moving code from Bash or Python to Go.

We recently did some golang migration [1] and happened to add the `cosa remote-session` pieces [2] after that migration. This makes it not easy to backport to ther branches. For a period of time let's use the latest COSA for our COSA pod for multi-arch builds instead of the versioned COSA. We will still use the versioned COSA on the remote so the actual building will happened with the correctly versioned COSA (substituted below and passed as an argument to `cosa remote-session create --image=`). [1] coreos/coreos-assembler#2919 [2] coreos/coreos-assembler#2979

We recently did some golang migration [1] and happened to add the `cosa remote-session` pieces [2] after that migration. This makes it not easy to backport to other branches. For a period of time let's use the latest COSA for our COSA pod for multi-arch builds instead of the versioned COSA. We will still use the versioned COSA on the remote so the actual building will happened with the correctly versioned COSA (substituted below and passed as an argument to `cosa remote-session create --image=`). [1] coreos/coreos-assembler#2919 [2] coreos/coreos-assembler#2979

cgwalters force-pushed the go-project branch from a7f77d2 to e05012d Compare June 16, 2022 20:14

jlebon previously approved these changes Jun 17, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

cgwalters dismissed jlebon’s stale review via eaba001 June 22, 2022 22:01

cgwalters force-pushed the go-project branch from e05012d to eaba001 Compare June 22, 2022 22:01

openshift-ci bot added the needs-rebase label Jun 25, 2022

cgwalters mentioned this pull request Jun 30, 2022

rework gangplank and pipelines #2860

Closed

cgwalters force-pushed the go-project branch from eaba001 to b3a2f76 Compare July 6, 2022 14:50

openshift-ci bot removed the needs-rebase label Jul 6, 2022

cgwalters force-pushed the go-project branch 2 times, most recently from 1704b08 to b0a840b Compare July 6, 2022 18:08

cgwalters force-pushed the go-project branch 8 times, most recently from c7dd06c to 8f94d61 Compare July 7, 2022 15:42

cgwalters force-pushed the go-project branch from 7b9cc0a to 05ed291 Compare July 11, 2022 19:24

cgwalters added a commit to cgwalters/os that referenced this pull request Jul 11, 2022

ci/prow-entrypoint: Ensure we have a qemu image for followup builds

ae2d28d

This should fix coreos/coreos-assembler#2919 (comment) Fixes: c6e7dd9

cgwalters mentioned this pull request Jul 11, 2022

ci/prow-entrypoint: Ensure we have a qemu image for followup builds openshift/os#889

Closed

travier pushed a commit to travier/os that referenced this pull request Jul 12, 2022

ci/prow-entrypoint: Ensure we have a qemu image for followup builds

e2ce5e9

This should fix coreos/coreos-assembler#2919 (comment) Fixes: c6e7dd9 Fixes: coreos/coreos-assembler#2973

travier mentioned this pull request Jul 12, 2022

CI: Re-enable buildextend testing for COSA CI openshift/os#890

Closed

travier pushed a commit to travier/os that referenced this pull request Jul 12, 2022

ci/prow-entrypoint: Ensure we have a qemu image for followup builds

c2bb12f

This should fix coreos/coreos-assembler#2919 (comment) Fixes: c6e7dd9 Fixes: coreos/coreos-assembler#2973

jlebon approved these changes Jul 12, 2022

View reviewed changes

dustymabe approved these changes Jul 13, 2022

View reviewed changes

dustymabe enabled auto-merge (rebase) July 13, 2022 14:42

dustymabe merged commit aa944b8 into coreos:main Jul 13, 2022

cgwalters mentioned this pull request Aug 15, 2022

build-extensions-container: add command to build the extensions container #2985

Merged

dustymabe mentioned this pull request Sep 16, 2022

[rhcos-4.11] support being initiated from a cosa remote-session #3108

Merged

Make /usr/bin/coreos-assembler a Go program, implement clean in Go #2919

Make /usr/bin/coreos-assembler a Go program, implement clean in Go #2919

Uh oh!

Conversation

cgwalters commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters commented Jun 14, 2022

Uh oh!

bgilbert commented Jun 14, 2022

Uh oh!

cgwalters commented Jun 14, 2022

Uh oh!

miabbott commented Jun 14, 2022

Uh oh!

cgwalters commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miabbott commented Jun 16, 2022

Uh oh!

travier commented Jun 17, 2022

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cgwalters commented Jul 6, 2022

Uh oh!

cgwalters commented Jul 6, 2022

Uh oh!

cgwalters commented Jul 6, 2022

Uh oh!

cgwalters commented Jul 7, 2022

Uh oh!

cgwalters commented Jul 7, 2022

Uh oh!

cgwalters commented Jul 11, 2022

Uh oh!

cgwalters commented Jul 11, 2022

Uh oh!

cgwalters commented Jul 12, 2022

Uh oh!

cgwalters commented Jul 12, 2022

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

jlebon Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

jlebon Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

jlebon commented Jul 12, 2022

Uh oh!

cgwalters commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dustymabe left a comment

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Jul 13, 2022

Uh oh!

cgwalters commented Jul 13, 2022

Uh oh!

travier commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Make `/usr/bin/coreos-assembler` a Go program, implement `clean` in Go #2919

Make `/usr/bin/coreos-assembler` a Go program, implement `clean` in Go #2919

cgwalters commented Jun 14, 2022 •

edited

Loading

cgwalters commented Jun 14, 2022 •

edited

Loading

cgwalters commented Jul 12, 2022 •

edited

Loading

travier commented Jul 25, 2022 •

edited

Loading