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

[#3810] forbid start when should be resumed #3965

Merged
merged 5 commits into from
Apr 30, 2024

Conversation

avikam
Copy link
Contributor

@avikam avikam commented Apr 24, 2024

Completing issue 3810.
This change adds a pre-flight check to the local installation mode, to verify no previous local config file exists, as an indication to a cluster that wasn't properly deleted.

@drc-infinyon
Copy link
Contributor

@digikata can you triage please.

@digikata
Copy link
Contributor

digikata commented Apr 26, 2024

The detection looks good, but probably needs a bats test.

This sequence fails to resume:

$ fluvio cluster start 
📝 Running pre-flight checks
    ✅ Local Fluvio is not installed
    ✅ Previous local fluvio installation not found
🎉 All checks passed!
✅ Local Cluster initialized
👤 Profile set
✅ SC Launched
🤖 Starting SPU: (1/1) 
✅ 1 SPU launched
🎯 Successfully installed Local Fluvio cluster

$ fluvio cluster start   # fails as intended
📝 Running pre-flight checks
    📝 Checking Fluvio Local Installation
found existing fluvio-run process. pid: 80138
found existing fluvio-run process. pid: 80136
    ❌ Check Fluvio Local Installation failed Local Fluvio cluster still running
    ❌ Check Clean Fluvio Local Installation failed Local Fluvio cluster wasn't deleted. Use 'resume' to resume created cluster or 'delete' before starting a new one
💔 Some pre-flight check failed!
Preflight check failed

$ fluvio cluster shutdown 
fluvio cluster shutdown
Removed spu monitoring socket: /tmp/fluvio-spu.sock
Uninstalled fluvio local components

$ fluvio cluster resume
 ✅ Local Fluvio is not installed
 ❌ Check Clean Fluvio Local Installation failed Local Fluvio cluster wasn't deleted. Use 'resume' to resume created cluster or 'delete' before starting a new one
 💔 Some pre-flight check failed!
 ❌ Resume failed with Preflight check failed

@avikam
Copy link
Contributor Author

avikam commented Apr 26, 2024

This sequence fails to resume:

working on a bat test. I think it might suffer from version discrepancy between (stable) and (dev), but valuable to have nonetheless.

@digikata
Copy link
Contributor

Yes, the test could be saved in an "incoming" directory that is maybe only run for new versions, then it can be moved in a later PR.

@morenol
Copy link
Contributor

morenol commented Apr 26, 2024

We do have a mechanism in bat tests to skip tests if running on stable, eg:

    if [ "$FLUVIO_CLI_RELEASE_CHANNEL" == "stable" ]; then
        skip "don't run on fluvio cli stable version"
    fi
    if [ "$FLUVIO_CLUSTER_RELEASE_CHANNEL" == "stable" ]; then
        skip "don't run on cluster stable version"
    fi

once we release a new stable we could remove that block

Copy link
Contributor

@digikata digikata left a comment

Choose a reason for hiding this comment

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

Looks great! It's a nice improvement in the local cluster shutdown/resume behavior @avikam

@digikata digikata added this pull request to the merge queue Apr 30, 2024
Merged via the queue into infinyon:master with commit 4726a5e Apr 30, 2024
106 checks passed
@avikam avikam deleted the avikam/resume-fail-start branch April 30, 2024 17:52
fraidev pushed a commit to fraidev/fluvio that referenced this pull request Apr 30, 2024
* forbid start when should be resumed

* fix resume regression

* bats

* bats - remove comments
fraidev pushed a commit to fraidev/fluvio that referenced this pull request May 23, 2024
* forbid start when should be resumed

* fix resume regression

* bats

* bats - remove comments
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.

4 participants