Skip to content
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

[BUG] Erroneous file globbing in cnf_manager - export_published_chart #1947

Closed
svteb opened this issue Apr 4, 2024 · 0 comments
Closed

[BUG] Erroneous file globbing in cnf_manager - export_published_chart #1947

svteb opened this issue Apr 4, 2024 · 0 comments
Assignees
Labels
bug Something isn't working

Comments

@svteb
Copy link
Collaborator

svteb commented Apr 4, 2024

Describe the bug
While running the database persistence spec test I ran into a rather unexpected bug with the TarClient module. The CNFManager.export_published_chart function uses this construct:

def self.export_published_chart(config, cli_args)
...
   tgz_name = "#{Helm.chart_name(helm_chart)}-*.tgz"
...

This causes the tar module to fail in case multiple versions of a helm chart archive are present in the testsuite directory.

To best describe the issue, let me provide you with an example. suppose the mysql sample cnf is going to be deployed, the TarClient module executes the following command to export the chart to the necessary folder in /cnfs/mysql/exported_chart:

tar -xvf mysql-*.tgz -C /cnfs/mysql/exported_chart ~>
tar -xvf mysql-9.0.1.tgz mysql-10.1.1.tgz mysql-10.1.2.tgz ... -C /cnfs/mysql/exported_chart

Supposing there are multiple mysql-*.tgz helm charts in the testsuite directory the tar command fails.

To Reproduce

I believe this issue might affect other spec tests/maybe even the main program, but I've only noticed it with the database_persistence spec test:

  1. export LOG_LEVEL=debug
  2. cd testsuite
  3. touch mysql-x.y.z.tgz or cp the actual mysql helm chart and rename it
  4. crystal spec spec/workload/state_spec.cr:43 | tee results/database_persistence.txt # database persistence spec test
  5. During the program runtime you should be capable of seeing that the /cnfs/mysql/exported_chart directory is not populated with files from the helm chart. The fact that the mysql statefulset successfully deploys is not indicative of this not being an error.
  6. The test fails, upon observing the results/database_persistence.txt file you may see log outputs like this:
I, [2024-04-04 08:10:48 +00:00 #1788708]  INFO -- cnf-testsuite: TarClient.untar current directory: /home/ubuntu/testsuite
I, [2024-04-04 08:10:48 +00:00 #1788708]  INFO -- cnf-testsuite: TarClient.untar command: tar  -xvf mysql-*.tgz -C /home/ubuntu/testsuite/cnfs/mysql/exported_chart
I, [2024-04-04 08:10:48 +00:00 #1788708]  INFO -- cnf-testsuite: TarClient.untar output:
I, [2024-04-04 08:10:48 +00:00 #1788708]  INFO -- cnf-testsuite: TarClient.untar stderr: tar: mysql-10.1.25.tgz: Not found in archive
tar: Exiting with failure status due to previous errors

Expected behavior
There should be some better mechanism to get the full archive name than file globbing on command line. I believe this could be somehow stored in the existing args/config variable that is being propagated around all the functions that work in this chain of deployment (view task "cnf_setup" and CNFManager.sample_setup) or the export_published_chart function could have some better logic to select a specific archive name.

Once this issue is address how will the fix be verified?

Regardless of how many archives of some cnf will be present in the testsuite directory, one of them will be exported (based on some logic that is to be implemented).

Additional context nagging
TarClient module (as well as all other modules) should raise an exception in case some vital command-line execution fails and this should somehow be handled in the code. But that is a different debate.

@svteb svteb added the bug Something isn't working label Apr 4, 2024
svteb added a commit to svteb/testsuite that referenced this issue May 23, 2024
Refs: cnti-testcatalog#1947
- Fixes the issue where multiple tar files caused the Tar module to fail due to globbing,
by selecting a single tgz file.
- There was a line that was supposed to delete the tgz file (no idea why): FileUtils.rm_rf(tgz_name), which
did not actually work due to the previously mentioned globbing, it was removed because it would
now cause the program to fail.

Signed-off-by: svteb <[email protected]>
svteb added a commit to svteb/testsuite that referenced this issue May 28, 2024
Refs: cnti-testcatalog#1947
- Backtracked on removal of: FileUtils.rm_rf(tgz_name), made it functional
through Dir.glob("#{Helm.chart_name(helm_chart)}-*.tgz").
Through this addition a state where multiple tgz archives are present is not
possible.
- Also added an exception in case a prior helm failure happens and no archive
is pulled (not sure whether this state can be reached).

Signed-off-by: svteb <[email protected]>
martin-mat pushed a commit that referenced this issue Jul 9, 2024
…2052)

* Fix: export_published_chart correctly detects and export tar file
Refs: #1947
- Fixes the issue where multiple tar files caused the Tar module to fail due to globbing,
by selecting a single tgz file.
- There was a line that was supposed to delete the tgz file (no idea why): FileUtils.rm_rf(tgz_name), which
did not actually work due to the previously mentioned globbing, it was removed because it would
now cause the program to fail.

Signed-off-by: svteb <[email protected]>

* Fix: Better handling of undesired states
Refs: #1947
- Backtracked on removal of: FileUtils.rm_rf(tgz_name), made it functional
through Dir.glob("#{Helm.chart_name(helm_chart)}-*.tgz").
Through this addition a state where multiple tgz archives are present is not
possible.
- Also added an exception in case a prior helm failure happens and no archive
is pulled (not sure whether this state can be reached).

Signed-off-by: svteb <[email protected]>

* Fix: minor refactor of tgz functions and clarified outputs
- Dir.glob functionality for getting tgz files in the working directory abstracted
into a function
- get_tgz_name function made more generic
- outputs made more explicit

Signed-off-by: svteb <[email protected]>

* Fix: Minor abstraction addition and use of warn log instead of info

Signed-off-by: svteb <[email protected]>

---------

Signed-off-by: svteb <[email protected]>
@svteb svteb closed this as completed Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant