Artifacts folder

[lucacome]

Bug Description

Every time I run preflight check container I need to change the artifacts folder with --artifacts=.

Error: could not write file to artifacts directory: /tmp/artifacts/results.json already exists

Version and Command Invocation

$ preflight --version
preflight version 1.4.2 <commit: f9cff772837132149df69f8ae251d3caf81c49ac>

Steps to Reproduce:

(How can we reproduce this?)

1. Run preflight check container with an image
2. Run preflight check container with an image a second time
3. See the error Error: could not write file to artifacts directory

Expected Result

The ideal would be for me to not have to specify the artifacts folder. I'm not even sure why that's needed.

Actual Result

I have to change the folder in the command line for every architecture of every image that I want to certify.

[acornett21]

Hi @lucacome This is working as designed, since we want to make sure the user clears out the artifacts folder for before each run to ensure we are using the latest information from the most recent run. Preflight by default writes to ./artifacts by defaults so if you clean that folder each run this shouldn't be an issue.

Preflight uses this folder to save off information gathered in the checks/policies that are then later used during the submission process to send to the backend system.

[lucacome]

Hi @acornett21 thanks for the explanation.

Maybe I'm missing some context, but I'm not sure how the artifacts folder is useful to the end user (like me).
Let's say I run preflight check and the outputs of the check are saved in artifacts. Then if I want to submit those results, since I don't have a specific command to do that without re-running the check, I just do preflight check --submit and get the error about the files already existing.
Another scenario is the one I described above about submitting multiple images/architectures (even without the two steps, just using the --submit flag right away). I would have to either change the folder name or delete it all together every time I run the command.

So in the end I'm not sure what benefit I'm getting from the artifacts folder (given that the results.json is also in the output of the command). Maybe I'm missing something and I'm supposed to use the files in the folder?

IMHO, a better experience (at least for me) is that if somebody uses --artifacts string means that they care about those files and should behave the same way it does today, but if they don't pass the flag it should default to a random folder in /tmp.

[jfrancin]

I very strongly disagree with this design. There is nothing in the real documentation (I don't mean some random "readme.md" file in a GitHub folder, but here: [1]) that discusses how multiple runs of Preflight will require cleaning out an artifacts folder.

You have changed the tool's behavior without the change being properly documented.

This change is now creating support cases.

I actually see no point in saving the artifacts folder; it should be created fresh each time, and any pre-existing one should be automatically deleted. The amount of time saved is minimal for most containers.

Please rescind the change.

An alternative way would be to only save the artifacts folder if the partner requests it via an --artifacts option as @lucacome mentioned; otherwise the folder's presence or absence should be invisible to the partner.

[1] https://access.redhat.com/documentation/en-us/red_hat_software_certification/8.53/html-single/red_hat_software_certification_workflow_guide/index

[komish]

This change is now creating support cases.

@jfrancin if able, could you describe/paraphrase what the support case's reported issue is, and what the recommended resolution is (most commonly)? I ask simply because it would seem the issue preflight detects, and its resolution should be derived from the error message it returns. And so I'm having trouble placing exactly what's landing a support case, given that the problem/cause here is, at least, somewhat clear.

Are there more aspects of this issue leading to/warranting the support case that I'm not seeing other than "I need to clean out the artifacts directory and I didn't need to do so before"?

You have changed the tool's behavior without the change being properly documented.

You're right. I generally supported this change because it, in my opinion, led to less inconsistency. That is, working with a fresh directory, to me, is preferable.

---

For what it's worth: with some upcoming changes in our code's organization, we will see similar collision issues with the artifacts directory that we'll look to resolve that will likely also resolve this issue.

[komish]

As a point of reference, I believe the issue is here:

openshift-preflight/certification/artifacts/artifacts.go

Lines 55 to 57 in 174eea6

	if err := afero.SafeWriteReader(appFS, fullFilePath, contents); err != nil {
	return fullFilePath, fmt.Errorf("could not write file to artifacts directory: %v", err)
	}

SafeWriteReader throws an error if the file already exists.

[jfrancin]

The cases basically say that they hit failures due to the errors being thrown by the existence of files in the artifacts directory. The resolution was to remove the files prior to re-running Preflight.

There are no other aspects other than what I reported. The problems are:

a) it's a behaviour change that wasn't documented (already discussed above);

b) it prevents doing an easy 'build-test-debug" workflow for partners working to weed out Preflight errors prior to running a final one with the --submit option; now they have to add an rm -r artfacts/ step. Yes, it's minor, but it introduces friction that wasn't there before, and it violates the principle of least surprise.

Perhaps as an alternative the created filenames in the artifacts directory could be timestamped in some way so that each run's results are independent and can coexist in the same directory? That way the 'freshness' of the directory is irrelevant, as every run will generate a uniquely-named set of artifact files.

[acornett21]

I just want to clear up a miss conception here, it's not just the results.json file that gets written to /artifacts. The reason why the error is always for results.json is since preflight create this file 'early' in execution (one could argue even before execution preflight create this file) to ensure that preflight has write access to disk. preflight needs write access in the checks to write artifacts to disk for two reasons:

• To trouble shoot checks further, ie DeployableByOLM writes a Subscription file which usually has the failure reason.
• To take data from a check and use that data to submit to pyxis, ie create container we need to store the container/rpm info.

So adding a timestamp wouldn't help, since we would not know which file to to pick for container/rpm/results. Having a clean/empty directory is ideal.

Like @komish mentioned, we are working on a large refactor that will hopefully also remove this pain point.

[komish]

Hey folks, for the time being, we've gone ahead and rolled back the behavior and cut a beta release so that you have something you can use. Please check out https://github.com/redhat-openshift-ecosystem/openshift-preflight/releases/tag/1.5.0-beta2.

[komish]

I believe with the merging of #866 that we can close this issue. Please re-open or create a new issue if there is anything further.