Extract buildinfo from yaml file and create content_manifest.json by gursewak1997 · Pull Request #2603 · coreos/coreos-assembler

gursewak1997 · 2021-12-08T07:33:15Z

We are adding the buildindfo data to the json file to be able to identify which repos were used to build RHCOS. The repo names used in manifest would match names in reposistory-to-CPE.json published by Red Hat

Got buildinfo from content_sets.yaml which resides under redhat-coreos repo and is copied over to src/config/. Extracted the repo names and mapped to their corresponding pulp repository IDs to add it to our content_manifest.json for buildinfo. The file content_manifest.json will be available under /usr/share/buildinfo/ after cmdlib.sh running is completed.

Issue: https://issues.redhat.com/browse/GRPA-3731
Discussion: #2540

miabbott

Looks like a good start, but I think there are some more changes needed

miabbott · 2021-12-08T14:23:56Z

 fi
 extra_compose_args+=("$parent_arg")

+# Convert the yaml file of content_sets to json format


I would keep this code behind a conditional that checks for the presence of the --content-sets argument. We don't need to run this for FCOS at this time.

Further, maybe it'd be cleaner to make this part of a function in cmdlib.sh and call out to it from prepare_compose_overlays when we detect the YAML file is present.

miabbott · 2021-12-08T15:03:17Z

 extra_compose_args+=("$parent_arg")

+# Convert the yaml file of content_sets to json format
+mkdir -p /srv/buildinfo/


Let's use "${workdir}"/tmp/buildinfo for this location

miabbott · 2021-12-08T15:09:33Z

+    mkdir -p ${tmp_overridesdir}/contentsetrootfs/usr/share/buildinfo/
+    echo `jq . /srv/buildinfo/content_manifest.json` > ${tmp_overridesdir}/contentsetrootfs/usr/share/buildinfo/content_manifest.json
+    echo -n "Committing ${tmp_overridesdir}/contentsetrootfs... "
+    cat ${tmp_overridesdir}/contentsetrootfs/usr/share/buildinfo/content_manifest.json
+    ostree commit --repo=$tmprepo --tree=dir=${tmp_overridesdir}/contentsetrootfs -b contentset 
+    cat >> "${override_manifest}" << EOF
+  - contentset
+EOF


I think this whole section of code should be conditional on the presence of content_manifest.json

I'm not sure how this code is supposed to work? The content_manifest.json is cat'ed, then an ostree commit is made, and the override manifest YAML doc has an empty -contentset entry added to it?

Yes, pretty much but cat content_manifest.json was not necessary. So basically just ostree commit the json file to /usr/share/ and adding -contentset entry to manifest yaml doc.

miabbott · 2021-12-08T15:16:35Z

Could you also expand the commit message to provide a bit more context on the "why?" for the change and link to the Jira card as part of it?

jlebon · 2021-12-08T18:37:22Z

 FORCE_IMAGE=
 SKIP_PRUNE=0
 VERSION=
+CONTENT_SETS=/srv/redhat-coreos/content_sets.yaml


Hmm, we shouldn't hardcode this path.

jlebon · 2021-12-08T18:37:43Z

 STRICT=
 rc=0
-options=$(getopt --options hft: --longoptions tag:,help,force,version:,parent:,parent-build:,delay-meta-merge,force-nocache,force-image,skip-prune,strict -- "$@") || rc=$?
+options=$(getopt --options hft: --longoptions tag:,help,force,version:,content_sets:,parent:,parent-build:,delay-meta-merge,force-nocache,force-image,skip-prune,strict -- "$@") || rc=$?


In general, cosa uses conventions. I was thinking more that instead of passing a --content-sets switch, cosa just does the right thing if it detects that it's present in the configdir.

jlebon · 2021-12-08T18:39:39Z

 fi
 extra_compose_args+=("$parent_arg")

+# Convert the yaml file of content_sets to json format


Further, maybe it'd be cleaner to make this part of a function in cmdlib.sh and call out to it from prepare_compose_overlays when we detect the YAML file is present.

miabbott · 2021-12-09T16:35:43Z

/retest

miabbott · 2021-12-09T16:36:59Z

FCOS CI is failing on the ShellCheck step

[2021-12-09T01:46:55.890Z] ./tests/check_one.sh src/cmdlib.sh src/.cmdlib.sh.shellchecked
[2021-12-09T01:46:56.452Z] 
[2021-12-09T01:46:56.452Z] In src/cmdlib.sh line 309:
[2021-12-09T01:46:56.452Z]     echo $(jq --argjson repos "$REPOS" '. += 
[2021-12-09T01:46:56.452Z]          ^-- SC2046: Quote this to prevent word splitting.
[2021-12-09T01:46:56.452Z]          ^-- SC2005: Useless echo? Instead of 'echo $(cmd)', just use 'cmd'.
[2021-12-09T01:46:56.452Z] 
[2021-12-09T01:46:56.452Z] For more information:
[2021-12-09T01:46:56.452Z]   https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt...
[2021-12-09T01:46:56.452Z]   https://www.shellcheck.net/wiki/SC2005 -- Useless echo? Instead of 'echo $(...
[2021-12-09T01:46:56.452Z] make: *** [Makefile:34: src/.cmdlib.sh.shellchecked] Error 1
script returned exit code 2```

jlebon · 2021-12-09T16:42:30Z

    else
        rm -vf "${local_overrides_lockfile}"
    fi
+    if [ -f /srv/redhat-coreos/content_sets.yaml ]; then


Should use configdir variable.

jlebon · 2021-12-09T16:44:31Z

+    if [ -f /srv/redhat-coreos/content_sets.yaml ]; then
+        create_content_manifest
+    fi
+    if [ -f "${workdir}/tmp/buildinfo/content_manifest.json" ]; then


Can we combine these? E.g.

if [ -e "${configdir}/content_sets.yaml" ]; then mkdir -p ... create_content_manifest /path/to/output.json ... fi

jlebon · 2021-12-09T16:46:58Z

+        echo -n "Committing ${tmp_overridesdir}/contentsetrootfs... "
+        ostree commit --repo="$tmprepo" --tree=dir="${tmp_overridesdir}"/contentsetrootfs -b contentset 
+        cat >> "${override_manifest}" << EOF
+  - contentset


This needs to go under ostree-layers. So we should probably instead just add it to the layers variable and then move down the block that outputs ostree-layers to after this.

jlebon · 2021-12-09T16:48:20Z

+create_content_manifest(){
+        # Convert the yaml file of content_sets to json format
+    mkdir -p "${workdir}"/tmp/buildinfo
+    yaml2json /srv/redhat-coreos/content_sets.yaml "${workdir}"/tmp/buildinfo/content_manifest.json


Same note here re. hardcoded path. Maybe simpler to have the caller to this function pass in the path to the YAML file.

Re. yaml2json, this is OK, though it doesn't look necessary. We could directly read YAML in the Python code.

jlebon · 2021-12-09T16:51:38Z

+repos=[];
+for i in data['repo_mapping']:
+    for j in data['repo_mapping'][i]:
+        if(j=='name'):repos.append(data['repo_mapping'][i][j])


This looks like it's outputting every repo in the file, but instead I think what we want is to parse the flattened manifest and only output the mapped repos which are used.

jlebon · 2021-12-09T16:55:01Z

+    # Add the build info to content_manifest.json
+    echo $(jq --argjson repos "$REPOS" '. += 
+    {
+    "metadata": {
+        "icm_version": 1,
+        "icm_spec": "https://raw.githubusercontent.com/containerbuildsystem/atomic-reactor/master/atomic_reactor/schemas/content_manifest.json",
+        "image_layer_index": 1
+    },
+    "content_sets": $repos,
+    "image_contents": []
+    }' "$workdir"/tmp/buildinfo/content_manifest.json) > "$workdir"/tmp/buildinfo/content_manifest.json


Seems like it'd be cleaner to build the file entirely in the Python bits.

jlebon · 2021-12-09T16:55:34Z

+    "content_sets": $repos,
+    "image_contents": []
+    }' "$workdir"/tmp/buildinfo/content_manifest.json) > "$workdir"/tmp/buildinfo/content_manifest.json
+    sed -i "s/\$ARCH/$(arch)/g" "$workdir"/tmp/buildinfo/content_manifest.json


Minor: the variable $arch should already be defined and accessible.

Changes landed

miabbott

Tested this locally with RHCOS and it worked great!

Saw the message about the missing repo:

Warning: No corresponding repo in repository-to-cpe.json for rhel-8-fast-datapath-aarch64                                                                                                                                                                                                                                     
Committing /srv/tmp/override/contentsetrootfs... e261cf0d7d1cf17ca3725f83d07da9aebfa3c66ea2d8641d36dd1ffc608d79eb

And the end result looked perfect.

$ cat /usr/share/buildinfo/content_manifest.json 
{
  "metadata": {
    "icm_version": 1,
    "icm_spec": "https://raw.githubusercontent.com/containerbuildsystem/atomic-reactor/master/atomic_reactor/schemas/content_manifest.json",
    "image_layer_index": 1
  },
  "content_sets": [
    "rhel-8-for-x86_64-baseos-eus-rpms__8_DOT_4",
    "rhel-8-for-x86_64-appstream-eus-rpms__8_DOT_4",
    "fast-datapath-for-rhel-8-x86_64-rpms",
    "rhocp-4.8-for-rhel-8-x86_64-rpms"
  ],
  "image_contents": []
}

The content_sets was missing the NFV repo, but I understand that is only being used for kernel-rt in the extensions. I wonder if we should just include all the repos by default?

I don't think we need to block on that question, as this gets us most of the way there.

gursewak1997 · 2021-12-15T17:29:30Z

The content_sets was missing the NFV repo, but I understand that is only being used for kernel-rt in the extensions. I wonder if we should just include all the repos by default?

I don't think including all the repos would be a good idea but I am open to it if needed.

gursewak1997 · 2021-12-15T17:56:49Z

Also, @miabbott are we still looking to add the symlink so that it is accessible as /root/buildinfo
If so, we just need to commit the buildinfo.conf file in https://github.com/openshift/os/pull/670/files

jlebon · 2021-12-16T21:27:16Z

+    echo "ostree-layers:" >> "${override_manifest}"
+    for layer in ${layers}; do
+        echo "  - ${layer}" >> "${override_manifest}"
+    done


Missing indentation.

jlebon · 2021-12-16T21:27:56Z

+        create_content_manifest "$configdir"/content_sets.yaml
+        mkdir -p "${tmp_overridesdir}"/contentsetrootfs/usr/share/buildinfo/
+        jq . "${workdir}/tmp/buildinfo/content_manifest.json" > "${tmp_overridesdir}/contentsetrootfs/usr/share/buildinfo/content_manifest.json"


Could make this clearer by passing the output path to create_content_manifest so it directly writes to the rootfs.

jlebon · 2021-12-16T21:29:58Z

+f.close();
+repos=[];
+
+for baseRepo in $base_repos:


Minor: base_repo would be more conventional.

jlebon · 2021-12-16T21:30:22Z

+        for i in data['repo_mapping'][baseRepo]:
+            if(i=='name'):


Hmm, why iterate over the dictionary? Isn't it directly accessible through data['repo_mapping'][baseRepo]['name']?

jlebon · 2021-12-16T21:31:19Z

+    if (baseRepo in data['repo_mapping'].keys()):
+        for i in data['repo_mapping'][baseRepo]:
+            if(i=='name'):
+                data['repo_mapping'][baseRepo][i]=data['repo_mapping'][baseRepo][i].replace('\$ARCH', '$(arch)');


Let's use a separate variable to mutate instead of mutating the source data?

jlebon · 2021-12-16T21:33:13Z

+                repos.append(data['repo_mapping'][baseRepo][i])
+    else:
+        # Printing this warning message till we figure out how to deal with these cases
+        print('Warning: No corresponding repo in repository-to-cpe.json for ' + baseRepo)


Shouldn't this say content_sets.yaml?

jlebon · 2021-12-16T21:38:31Z

+                repos.append(data['repo_mapping'][baseRepo][i])
+    else:
+        # Printing this warning message till we figure out how to deal with these cases
+        print('Warning: No corresponding repo in repository-to-cpe.json for ' + baseRepo)


I wonder if we should tweak the semantics a bit: we include those repos that aren't in repository-to-cpe.json in content_sets.yaml but leave their name as "". Then here, we can warn and skip if the name is empty, otherwise explicitly error out. This ensures that if the set of manifest repo names changes, we're not just going to forget to update content_sets.yaml.

Another way to say this: we require that every repo in the manifest must have an entry in content_sets.yaml.

Agree with it.
I was thinking we could have two different warning messages;

one for the repos with "" as their name in yaml file

and another one if their is no entry for a base_repo in the yaml file itself. This would help us to add an entry to the YAML file in case we miss one in the yaml file in the future.

jlebon · 2021-12-16T21:43:31Z

+        for i in data['repo_mapping'][baseRepo]:
+            if(i=='name'):
+                data['repo_mapping'][baseRepo][i]=data['repo_mapping'][baseRepo][i].replace('\$ARCH', '$(arch)');
+                repos.append(data['repo_mapping'][baseRepo][i])


Ideally here we'd be checking that the baseurls match. This would require parsing the repo files though. We can skip this for now.

jlebon · 2021-12-16T21:44:39Z

+    mkdir -p "${workdir}"/tmp/buildinfo
+    base_repos=$(jq .repos "${manifest_tmp_json}")
+    # Make an empty json file in tmp/buildinfo/
+    echo {} > "$workdir"/tmp/buildinfo/content_manifest.json


Shouldn't need this since we should always be creating a JSON file by the end.

jlebon · 2021-12-16T21:48:20Z

Also, maybe for the commit message, something like: "cmd-build: add support for content_sets.yaml"? Also should include a body containing a bit of context on why we're doing this and if needed links to more context.

jlebon

Some minor comments, but mostly LGTM! Looks like

Also should include a body containing a bit of context on why we're doing this and if needed links to more context.

still needs to be addressed.

jlebon · 2021-12-17T19:37:46Z

        fi
    done
    rm -f "${manifest_tmp_json}"
-


Minor: spurious line deletion hunk.

jlebon · 2021-12-17T19:38:46Z

+    python3 -c "
+import json, yaml;
+# Open the yaml and load the data
+f = open('$1')


Instead of using $1 and $2 like this, let's declare some local variables upfront at the beginning of the function with clearer names.

jlebon · 2021-12-17T19:40:12Z


+create_content_manifest(){
+    mkdir -p "${workdir}"/tmp/buildinfo
+    rpm-ostree compose tree --repo="${tmprepo}" --print-only "${manifest}" > "${manifest_tmp_json}"


Let's set manifest_tmp_json ourselves here as a local variable (and the manifest_tmp_json in prepare_build should probably also be local).

Alternatively, we can formalize this as part of prepare_build and not delete the treefile like you had before but name the variable something like flattened_manifest and explicitly export it the same way we export manifest. Then here we can reuse it instead of rerunning rpm-ostree compose tree. Anyway, not a blocker!

jlebon · 2021-12-17T19:50:09Z

+            print('Warning: No corresponding repo in repository-to-cpe.json for ' + base_repo)
+    else:
+        # Warning message for repositories with no entry in content_sets.yaml
+        print('Warning: No corresponding entry in content_sets.yaml for ' + base_repo)


What I meant here is that we explicitly error out, but OK with this too to start. We can always tighten things later.

Actually, this is probably better for the developer case. So it's a good default and then we could add a switch for more stricter handling in production pipelines, a bit like how it is for lockfiles.

Keeping as it for now.

jlebon · 2021-12-17T19:51:02Z

 cmdlib.import_ostree_commit('${workdir:-$(pwd)}/tmp/repo', builddir, buildmeta)
 ")
-}
+}


Minor: looks like we're eating the file's final newline here.

jlebon

LGTM, nice work!

jlebon · 2021-12-17T21:50:42Z

+    source_file=$1
+    destination=$2


Minor: these should be local. Not worth the respin though.

-Adding the buildindfo data to the json file to be able to identify which repos were used to build RHCOS. These changes will map the repos used to their respective pulp repository IDs from content_sets.yaml. - Issue: https://issues.redhat.com/browse/COS-54

Make a helper for committing OSTree layers which contains all the canonicalization flags. Notably `--timestamp` which ensures that we get a consistent checksum for the same source git commit. Fixes: openshift/os#712 Fixes: coreos#2603

Make a helper for committing OSTree layers which contains all the canonicalization flags. Notably `--timestamp` which ensures that we get a consistent checksum for the same source git commit. Fixes: openshift/os#712 Fixes: #2603

Make a helper for committing OSTree layers which contains all the canonicalization flags. Notably `--timestamp` which ensures that we get a consistent checksum for the same source git commit. Fixes: openshift/os#712 Fixes: coreos#2603

Make a helper for committing OSTree layers which contains all the canonicalization flags. Notably `--timestamp` which ensures that we get a consistent checksum for the same source git commit. Fixes: openshift/os#712 Fixes: #2603

Make a helper for committing OSTree layers which contains all the canonicalization flags. Notably `--timestamp` which ensures that we get a consistent checksum for the same source git commit. Fixes: openshift/os#712 Fixes: coreos#2603 (cherry picked from commit 3fac918) dustymabe: This cherry pick needed manual merge conflict resolution. No contentsets in 4.9.

Make a helper for committing OSTree layers which contains all the canonicalization flags. Notably `--timestamp` which ensures that we get a consistent checksum for the same source git commit. Fixes: openshift/os#712 Fixes: #2603 (cherry picked from commit 3fac918) dustymabe: This cherry pick needed manual merge conflict resolution. No contentsets in 4.9.

miabbott previously requested changes Dec 8, 2021

View reviewed changes

jlebon reviewed Dec 8, 2021

View reviewed changes

jlebon reviewed Dec 9, 2021

View reviewed changes

gursewak1997 linked an issue Dec 13, 2021 that may be closed by this pull request

Publish content_manifest to include repos used to build RHCOS #2540

Closed

gursewak1997 requested review from jlebon and miabbott December 13, 2021 16:58

openshift-ci Bot added the needs-rebase label Dec 14, 2021

coreos deleted a comment from openshift-ci Bot Dec 14, 2021

openshift-ci Bot removed the needs-rebase label Dec 14, 2021

gursewak1997 closed this Dec 14, 2021

gursewak1997 reopened this Dec 14, 2021

miabbott previously approved these changes Dec 15, 2021

View reviewed changes

jlebon reviewed Dec 16, 2021

View reviewed changes

gursewak1997 dismissed miabbott’s stale review via 2ff8097 December 17, 2021 18:29

gursewak1997 requested a review from jlebon December 17, 2021 19:32

jlebon reviewed Dec 17, 2021

View reviewed changes

jlebon previously approved these changes Dec 17, 2021

View reviewed changes

gursewak1997 dismissed jlebon’s stale review via 0268bae December 17, 2021 22:01

jlebon approved these changes Dec 17, 2021

View reviewed changes

gursewak1997 merged commit 19531a6 into coreos:main Dec 20, 2021

jlebon mentioned this pull request Feb 10, 2022

cmdlib: consistently commit layers with same options #2703

Merged

jlebon mentioned this pull request Aug 26, 2022

Add support for copying yum repos from separate git repo #3036

Merged

Conversation

gursewak1997 commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miabbott left a comment

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!

miabbott commented Dec 8, 2021

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!

miabbott commented Dec 9, 2021

Uh oh!

miabbott commented Dec 9, 2021

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!

miabbott left a comment

Choose a reason for hiding this comment

Uh oh!

gursewak1997 commented Dec 15, 2021

Uh oh!

gursewak1997 commented Dec 15, 2021

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!

jlebon commented Dec 16, 2021

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gursewak1997 commented Dec 8, 2021 •

edited

Loading