report correct version when multiple images invoked #327
Conversation
|
Just for reference: #129 is a PR introducing the |
It's unclear whether the mentioned version corresponds to the must-gather image version or to the OCP version. Worth updating those lines to be explicit the version corresponds to must-gather version. |
collection-scripts/version
Outdated
| # if version not found, fallback to imageID | ||
| [ -z "${version}" ] && version=$(oc status | grep '^pod.*runs' | sed -r -e 's/^pod.*runs //') | ||
| # get gather container image and imageID | ||
| read -r image imageID < <(oc get pod ${POD_NAME} -o jsonpath='{range .status.containerStatuses[?(@.name=="gather")]}{.image} {.imageID}{end}') |
There was a problem hiding this comment.
Assuming the script is not run outside a container, it's ok to read POD_NAME variable (always set in the must-gather pod spec). Nevertheless:
- reading
POD_NAMEenv creates a new dependency on thepkg/cli/admin/mustgathercode (when thePOD_NAMEvariable is removed, the version will fall back into "Unknown") .status.containerStatusesdoes not always have to be set at the time when theversioncommand is invoked
What about to update the building pipeline to inject the must-gather version directly? So version() can be a simple reading of a file?
There was a problem hiding this comment.
@sanchezl I am a bit indifferent to the solution here. What do you think is best/easiest?
There was a problem hiding this comment.
@sanchezl were you or @ingvagabund looking to make updates to this PR to close on this thread?
There was a problem hiding this comment.
Looks like I have the following available (with example values from 4.12.8):
"BUILD_RELEASE=202210242028.p0.ge105f12.assembly.stream",
"BUILD_VERSION=v4.12.0",
"OS_GIT_MAJOR=4",
"OS_GIT_MINOR=12",
"OS_GIT_PATCH=0",
"OS_GIT_TREE_STATE=clean",
"OS_GIT_VERSION=4.12.0-202210242028.p0.ge105f12.assembly.stream-e105f12",
"SOURCE_GIT_TREE_STATE=clean",
"OS_GIT_COMMIT=e105f12
Looks like OS_GIT_VERSION might be appropriate.
There was a problem hiding this comment.
@sanchezl , is it possible with that OS_GIT_VERSION to know the exact version/commit of the must-gather scripts (https://github.com/openshift/must-gather/commits/master/collection-scripts) used?
|
As per last comment in #314 , the current |
|
@oarribas the version file here; is more about identifying what; image (or collector was used) and not what 'oc client' triggered the collection. Thus I don't think its is of a huge significance or issue that the client version isn't denoted. |
sanchezl
force-pushed
the
image-version
branch
from
a7a7f5d
to
ce8eca9
Compare
| [ -z "${version}" ] && version="Unknown" | ||
|
|
||
| echo ${version} | ||
| if [[ ! -z $OS_GIT_VERSION ]] ; then |
There was a problem hiding this comment.
OS_GIT_VERSION is defined only during the building phase. Once built, the env gets unpopulated. What about to inject the env value in https://github.com/openshift/must-gather/blob/master/Dockerfile.rhel7? E.g. re-generating the version file from scratch?
There was a problem hiding this comment.
The following are embedded into the images during build, and as such are available when the image is run:
'OS_GIT_MAJOR': major_version,
'OS_GIT_MINOR': minor_version,
'OS_GIT_PATCH': patch_version,
'OS_GIT_VERSION': f'{major_version}.{minor_version}.{patch_version}{release_suffix}',
'OS_GIT_TREE_STATE': 'clean',
'SOURCE_GIT_TREE_STATE': 'clean',
'BUILD_VERSION': version,
'BUILD_RELEASE': release if release else '',
You can download the must-gather.tar from one of the jobs on this PR, and see that the version file contains:
openshift/must-gather
4.13.0-202211071433.p0.g63e9a04.assembly.stream-63e9a04
There was a problem hiding this comment.
Following https://github.com/openshift/enhancements/blob/master/enhancements/oc/must-gather.md#must-gather-images and the version string major.minor.micro.qualifier. I wonder what the expected mapping should be: major=4, minor=13, micro=0-202211071433, qualifier=p0.g63e9a04.assembly.stream-63e9a04? Compared to https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_must-gather/327/pull-ci-openshift-must-gather-master-images/1597782051325480960/artifacts/ci-operator-step-graph.json:
- OS_GIT_MAJOR=4
- OS_GIT_MINOR=13
- OS_GIT_PATCH=0
- BUILD_RELEASE=202211071433.p0.g63e9a04.assembly.stream
We might as well update the version string in enhancements/oc/must-gather.md to major.minor.micro-qualifier with major/minor/micro as [0-9]+ so the parts turn into major=4, minor=13, micro=0, qualifier=202211071433.p0.g63e9a04.assembly.stream-63e9a04.
There was a problem hiding this comment.
@ingvagabund I've updated the enhancement to specify the SemVer format (original intention) used elsewhere in OpenShift (vs. the OSGi-type format specified there originally).
There was a problem hiding this comment.
With openshift/enhancements#1294 merged we are good here. @soltysh would you please approve the PR?
|
@sferich888 the current code (not this PR) of the |
|
/lgtm |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ingvagabund, sanchezl, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@sanchezl: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Fix the generation of the
/must-gather/versionfile such that if multiple must-gather images are invoked at once, the only the version of the currently running image will be recorded in the/must-gather/versionfile.If a semantic-like version cannot be parsed from the image's
repository:tagname (note that:latestis not a semantic-like version), use the imageID (typically arepository@digest) value.