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

[Merged by Bors] - Add specific log_dir creation error #2425

Closed
wants to merge 9 commits into from

Conversation

joshchngs
Copy link
Contributor

Previous implementation simply prints:
0: Fluvio cluster error
1: Permission denied (os error 13)

...which gives no information regarding which operation caused the
error, or even to which path the error relates.

This commit adds an interstitial error to the chain, which prints:
1: An error occurred creating the cluster log directory "/usr/local/var/log/fluvio"

@joshchngs
Copy link
Contributor Author

@nicholastmosher I hit this issue because my user doesn't have any write permissions inside /usr/local.
I'm on Apple Silicon, MacOS 12.4.

I don't have a quick way to confirm with a fresh install, but I think this is the new default permission set on arm64.
Question: what's the reason for the different default log_dir under macos, and is it still valid?

@sehz sehz requested a review from tjtelan June 20, 2022 21:01
Copy link
Contributor

@tjtelan tjtelan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a quick way to confirm with a fresh install, but I think this is the new default permission set on arm64.

@joshchngs Is there any way you can write a simple test that uses the CLI to throw the error and validates your error message matches what you set?

Please add it to the Mac CI. This is a filesystem permissions issue, not specifically a MacOS related issue. But it will be the easiest area for you to add CLI tests against a local cluster.

Copy this job as a base for your local CLI test

local_cluster_test:

Replace the job name, modify the job to set your test conditions and run your test

- name: Run smoke-test
timeout-minutes: 5
run: |
date
make smoke-test-local

@joshchngs joshchngs force-pushed the macos-log-dir branch 4 times, most recently from 0424682 to 05c1edb Compare June 27, 2022 00:45
@joshchngs
Copy link
Contributor Author

That's done. It's a fairly trivial test which comes with 10 minutes of boilerplate docker/k8s setup. I'd suggest rolling it into the existing local cluster setup as an additional step if the overhead becomes a problem.

@joshchngs
Copy link
Contributor Author

Alright, that's ready to merge from my end. Pushed a CI test for the default case, which seems OK on the x86_64 macos-12 runner, so I unpushed that commit. So, either something else changed /usr permissions on my setup or it's actually different on arm64. At least if somebody else hits it there is a more informative error.

Copy link
Contributor

@tjtelan tjtelan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last ask before merge: Can you add a reference to this fix in CHANGELOG.md?

Otherwise, LGTM, thanks!

Previous implementation simply prints:
	0: Fluvio cluster error
	1: Permission denied (os error 13)

...which gives no information regarding which operation caused the
error, or even to which path the error relates.

This commit adds an interstitial error to the chain, which prints:
	1: An error occurred creating the cluster log directory "/usr/local/var/log/fluvio"
This job will run on MacOS 12 (different to the existing job).
Checks fail when a cluster has already been started (Helm chart & metadata is already present).
It doesn't matter in this case, but it causes the CLI to bail before the code we're testing.
This is set automatically by Actions. We very much don't want that here,
since we *expect* part of the script to fail.
This is to debug the job. It looks like either a caching issue with
Artifacts, or I'm misunderstanding how they should work.

The test is failing on CI - apparently correctly because the output
doesn't include the required message. However the artifact published
for this branch behaves correctly when run locally.

It should be OK to leave the stderr output in, and may be useful
in the future.
Although we're not testing anything to do with k8s, the CLI hits
a k8s error before the one we're interested in, so it bails out
before the code under test can run.
This shouldn't make any functional difference, but it *does* mean
that any issues with logging will be hit before k8s errors.
@joshchngs
Copy link
Contributor Author

Done - against 0.9.30 (rebased on master)

@sehz
Copy link
Contributor

sehz commented Jun 28, 2022

@sehz
Copy link
Contributor

sehz commented Jun 28, 2022

For homebrew users, we didn't encounter this issue since we probably fix the permission problem a long time ago and totally forgotten about it.

@sehz sehz added this to the 0.9.30 milestone Jun 28, 2022
@sehz
Copy link
Contributor

sehz commented Jun 28, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jun 28, 2022
Previous implementation simply prints:
	0: Fluvio cluster error
	1: Permission denied (os error 13)

...which gives no information regarding which operation caused the
error, or even to which path the error relates.

This commit adds an interstitial error to the chain, which prints:
	1: An error occurred creating the cluster log directory "/usr/local/var/log/fluvio"
@bors
Copy link

bors bot commented Jun 28, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title Add specific log_dir creation error [Merged by Bors] - Add specific log_dir creation error Jun 28, 2022
@bors bors bot closed this Jun 28, 2022
@joshchngs joshchngs deleted the macos-log-dir branch June 28, 2022 13:35
morenol pushed a commit to morenol/fluvio that referenced this pull request Jun 30, 2022
Closes infinyon#2437

* [x] Increment patch version in [`VERSION`](https://github.com/infinyon/fluvio/blob/master/VERSION) file
* [x] Update [`CHANGELOG`](https://github.com/infinyon/fluvio/blob/master/CHANGELOG.md) with replacement of the `UNRELEASED` date for most recent release
* [x] Add a new heading in [`CHANGELOG`](https://github.com/infinyon/fluvio/blob/master/CHANGELOG.md) with for the next release (Use heading level 2)

Add specific log_dir creation error (infinyon#2425)

Previous implementation simply prints:
	0: Fluvio cluster error
	1: Permission denied (os error 13)

...which gives no information regarding which operation caused the
error, or even to which path the error relates.

This commit adds an interstitial error to the chain, which prints:
	1: An error occurred creating the cluster log directory "/usr/local/var/log/fluvio"

Re-added producer config to connector yaml (infinyon#2444)

This is the non breaking changes changes from infinyon#2426.

Add VecOrString for ManagedConnectorSpec parameters

cargo fmt and clippy

Verify some portion of backward compatability

backward compatible encoding for manage connector parameters

fix serialize issue

cargo fmt

fix tests

Updates from comments

Updated watch version

Updates from comments

fix ci
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants