Add warning message to docker boot #599

fearful-symmetry · 2021-11-24T00:24:49Z

So I learned (or forgot, and then remembered) the hard way that if you don't run elastic-stack up in an integrations directory, it won't add any custom-built integrations.

This just adds a little warning message if it can't find the development directory. However, considering the opportunity for weird silent failures here, I wonder if we should fail out if found == false, perhaps with a user override.

elasticmachine · 2021-11-24T01:00:15Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-03-17T09:16:19.687+0000
Duration: 21 min 25 sec

Test stats 🧪

Test	Results
Failed	0
Passed	582
Skipped	1
Total	583

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

mtojek · 2021-11-24T06:14:56Z

internal/stack/boot.go

@@ -41,6 +41,8 @@ func BootUp(options Options) error {
 		if err != nil {
 			return errors.Wrap(err, "copying package contents failed")
 		}
+	} else {
+		fmt.Println("WARNING: no custom build packages directory found. Any locally built integrations will not be installed.")


There is a message logged in L39 informing about used package directories. Do you see this as insufficient/less informative/improvement needed?

The reason I'm asking is that I'm concerned about users booting the stack without the intention of loading any extra integrations. The warning might be confusing for them.

Yeah, I agree with that, it can be confusing for users not developing packages.

Maybe we can still show a message about the content of the registry, but not as a warning.

Suggested change

fmt.Println("WARNING: no custom build packages directory found. Any locally built integrations will not be installed.")

fmt.Println("No custom build packages directory found.")

And the messages below also clarify the packages loaded. I think this would be enough info.

Another thing that could be considered to help in this use case is that when you run elastic-package build, it somehow detects if the package is being served by the current registry if any is running, and if not, it prints a message there asking to restart the stack from this directory.

And yet another thing we can do to help on these scenarios is to make the registry automatically able to reindex packages without needing to be restarted.

Yah, I think we need something that prints a message in the negative case and actually reports when something isn't working as expected.

It is working as expected as the process wasn't started in the integrations repo. Maybe we should rephrase this warning into something like:

logger.Info("Local packages won't be included as the elastic-package didn't find any built packages.").

WDYT?

jsoriano

Oh, I had commented but forgot to submit 🤦

jsoriano · 2021-11-24T10:43:10Z

internal/stack/boot.go

@@ -41,6 +41,8 @@ func BootUp(options Options) error {
 		if err != nil {
 			return errors.Wrap(err, "copying package contents failed")
 		}
+	} else {
+		fmt.Println("WARNING: no custom build packages directory found. Any locally built integrations will not be installed.")


Yeah, I agree with that, it can be confusing for users not developing packages.

Maybe we can still show a message about the content of the registry, but not as a warning.

Suggested change

fmt.Println("WARNING: no custom build packages directory found. Any locally built integrations will not be installed.")

fmt.Println("No custom build packages directory found.")

And the messages below also clarify the packages loaded. I think this would be enough info.

Another thing that could be considered to help in this use case is that when you run elastic-package build, it somehow detects if the package is being served by the current registry if any is running, and if not, it prints a message there asking to restart the stack from this directory.

And yet another thing we can do to help on these scenarios is to make the registry automatically able to reindex packages without needing to be restarted.

mtojek · 2021-11-24T18:37:22Z

And yet another thing we can do to help on these scenarios is to make the registry automatically able to reindex packages without needing to be restarted.

I can give some historical context:

I tried to mount built packages into Docker container so that when the end-user rebuilds packages, auto-discovery will reindex all files. It was problematic as it was reindexing really slowly when the directory was mounted (a few seconds vs a few minutes).
I decided to rebuild the Docker image for the Package Registry and restart it. It went well until Kibana enabled package caching, which means that if Kibana precaches the developed package, you have to restart also the Kibana :) or artificially change the package version.
I do believe that there are improvements possible, but TBH nothing would be better than installing packages directly in Kibana instead and simply forgetting about running the Registry here. It would really simplify this workflow.

I'm waiting for option 3. to be delivered :)

jsoriano · 2021-11-24T18:41:06Z

I'm waiting for option 3. to be delivered :)

++

Everything else are workarounds :D

fearful-symmetry · 2021-11-24T20:51:36Z

So, my concern/paranoia here is mostly about silently bypassing something in the local integrations build, i.e

Make a change in an integration
Build it, start up the stack outside the integration directory
Test it without noticing that you're not running the version that you were working on.

This is kind of a subtle failure mode, and it leaves more leeway for user oversight than I'd like.

Is there a reason why the local build dir -> .elastic-package/ build dir copy step happens during the docker bootup? If we moved that step to elastic-package build it would render this kind of a moot issue @jsoriano / @mtojek

mtojek · 2021-11-24T21:50:18Z

Is there a reason why the local build dir -> .elastic-package/ build dir copy step happens during the docker bootup? If we moved that step to elastic-package build it would render this kind of a moot issue @jsoriano / @mtojek

elastic-package build is a package-oriented operation, which shouldn't trigger any docker builds as these aren't related. In the nearest future, we plan to get rid of rebuilding the Docker image for Package Registry, so it will simplify the workflow.

Considering your suggestion, If you want to build 100 packages, does it mean you have to rebuild the Docker image 100 times? Another analogy, when you are building a binary out of a Go code using go build, or a program in C with gcc, it doesn't build any Docker image as it's unaware of the distribution form.

fearful-symmetry · 2021-11-29T17:41:47Z

elastic-package build is a package-oriented operation, which shouldn't trigger any docker builds as these aren't related.

I might be mixing stuff up, but the part of elastic-package stack up that copies the build dir doesn't need to trigger any builds or rebuilds? We can do just the copying in the package-build stage without triggering any kind of docker operation?

mtojek · 2021-11-30T09:52:30Z

I might be mixing stuff up, but the part of elastic-package stack up that copies the build dir doesn't need to trigger any builds or rebuilds?

No, it does trigger the docker build operation for a custom Package Storage image (see code ref).

We considered alternative approaches in the past. For example, mounting a volume resulted in a slow start of the Package Registry, and using a filewatcher to refresh the Package Registry's internal state wasn't stable. As I already said, the flow will be simplified if Kibana allows for installing packages directly without any interaction with Package Registry.

fearful-symmetry · 2021-11-30T17:25:18Z

@mtojek What I'm referring to is the exact copy operation here:

elastic-package/internal/stack/boot.go

Lines 38 to 44 in 6215ed4

    
           if found { 
        
           	fmt.Printf("Custom build packages directory found: %s\n", buildPackagesPath) 
        
           	err = files.CopyAll(buildPackagesPath, stackPackagesDir.PackagesDir()) 
        
           	if err != nil { 
        
           		return errors.Wrap(err, "copying package contents failed") 
        
           	} 
        
           }

Is there a reason that needs to be in the BootUp phase? Couldn't that simply be copied to the BuildPackage() function, and then we'd just build the container normally during BootUp ?

mtojek · 2021-11-30T20:19:18Z

Then, it will be misleading for users who want to build a package, but prefers to boot a clean stack. I don't see an intuitive workaround for this. Do you see any other options, @jsoriano?

fearful-symmetry · 2021-11-30T22:05:47Z

I feel like this behavior should be explicit rather than implicit. If a user wants to boot a clean stack or development stack, there should be a CLI flag that controls that behavior, rather than depending on something more subtle like the development directory. Also keep in mind that if someone runs elastic-package stack up -v, the verbose log messages will quickly swallow some of these initialization messages.

mtojek · 2021-11-30T23:32:40Z

It's implicit as it's natural to stay in the package directory while developing it. If you need to add files to the Git index, you need to be in the working directory.

Also keep in mind that if someone runs elastic-package stack up -v, the verbose log messages will quickly swallow some of these initialization messages.

Would a special logging tag help here? For example: [development stack] or [clean stack]?

As I said before, this behavior will change when we eventually support direct package installation in Kibana and drop requirement for Package Registry.

jsoriano · 2021-12-01T15:51:12Z

I don't think that we should add more implicit behaviours to elastic-package build or elastic-package stack.

I agree with Alex that the current behaviour is confusing: you need to remember to run elastic-package stack up from the directory where the integration lives and you need to run it again after build so the registry includes the package. This is a usual problem new package developers find.

If you need to add files to the Git index, you need to be in the working directory.

This is partially true. There may be many reasons why the terminal where you run elastic-package stack is not in the proper directory. You may be modifying some files in a package, then you open a new terminal to start the stack to try it, and this terminal may be opened in $HOME, and then you start a stack without the package you are trying to test. Or you may be using an IDE for everything, including managing Git.

Log messages (even big red warnings) are still a partial solution. They may help sometimes, but you can easily overlook them.

As mentioned the ideal solution would be to have elastic/kibana#70582, and when we have it, we can probably make elastic-package install to explicitly install the current package. In the meantime any other solution is going to be a compromise.

Maybe we should think ahead in the UX based on elastic-package install. It could be implemented this way now:

It stops the registry started in the stack maintained with elastic-package stack (if no stack is started it could fail explicitly mentioning that).
It starts a registry that includes the package to be installed.
It installs the package.

Then it doesn't matter how the stack was started, the installation is going to install what is expected by the developer.

We would replace the steps of running elastic-package stack up to restart the registry with the new package and manually installing the package with elastic-package install, that is more explicit for the operation that the developer is trying to do.

Once elastic/kibana#70582 is available, we use this API instead of restarting the registry, but the developer UX is the same.

fearful-symmetry · 2021-12-01T21:15:23Z

+1 on @jsoriano 's idea, an install command like that would be relatively explicit, and make the develop/test workflow a tad easier too.

mtojek · 2021-12-02T10:31:40Z

Keep in mind that it will be still confusing as there is a package cache in Kibana.

Consider the following scenario:

User is developing the package foo-1.2.3.
User builds the package with elastic-package build.
User boots the Elastic stack, the stack contains the new package. User orders Kibana to install it, Kibana caches the package.

Unfortunately, there is a bug and user needs to build the package again (same version).

User applies a bugfix and builds the package.
User runs elastic-package install, which rebuilds the Package Registry, installs the package in Kibana, BUT....

... Kibana installs the same old, cached package and the user is confused now.

jsoriano · 2021-12-02T10:54:44Z

... Kibana installs the same old, cached package and the user is confused now.

I guess that this problem also exists with current implementation?

mtojek · 2021-12-02T10:59:26Z

Yes. In such cases, it's recommended to take down the stack or artificially bump up the package version.

My point is the solution you proposed won't fix the entire problem.

EDIT:

With this solution, we may end up with issues like: "install" command doesn't work.

mtojek · 2022-04-01T08:04:04Z

The PR is stale, let's close it for now.

add file

57bc7a1

fearful-symmetry requested review from a team November 24, 2021 00:24

fearful-symmetry self-assigned this Nov 24, 2021

mtojek reviewed Nov 24, 2021

View reviewed changes

mtojek requested a review from jsoriano November 24, 2021 06:16

jsoriano reviewed Nov 24, 2021

View reviewed changes

mtojek closed this Apr 1, 2022

jsoriano mentioned this pull request May 9, 2022

Add config option to deploy custom elastic-agents as test services #786

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning message to docker boot #599

Add warning message to docker boot #599

fearful-symmetry commented Nov 24, 2021

elasticmachine commented Nov 24, 2021 •

edited

Loading

Build stats

Test stats 🧪

mtojek Nov 24, 2021 •

edited

Loading

jsoriano Nov 24, 2021

fearful-symmetry Nov 24, 2021

mtojek Nov 24, 2021

jsoriano left a comment

jsoriano Nov 24, 2021

mtojek commented Nov 24, 2021

jsoriano commented Nov 24, 2021

fearful-symmetry commented Nov 24, 2021 •

edited

Loading

mtojek commented Nov 24, 2021

fearful-symmetry commented Nov 29, 2021

mtojek commented Nov 30, 2021

fearful-symmetry commented Nov 30, 2021

mtojek commented Nov 30, 2021 •

edited

Loading

fearful-symmetry commented Nov 30, 2021

mtojek commented Nov 30, 2021 •

edited

Loading

jsoriano commented Dec 1, 2021 •

edited

Loading

fearful-symmetry commented Dec 1, 2021

mtojek commented Dec 2, 2021

jsoriano commented Dec 2, 2021

mtojek commented Dec 2, 2021 •

edited

Loading

mtojek commented Apr 1, 2022

	fmt.Println("WARNING: no custom build packages directory found. Any locally built integrations will not be installed.")
	fmt.Println("No custom build packages directory found.")

Add warning message to docker boot #599

Add warning message to docker boot #599

Conversation

fearful-symmetry commented Nov 24, 2021

elasticmachine commented Nov 24, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

mtojek Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

jsoriano Nov 24, 2021

Choose a reason for hiding this comment

fearful-symmetry Nov 24, 2021

Choose a reason for hiding this comment

mtojek Nov 24, 2021

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano Nov 24, 2021

Choose a reason for hiding this comment

mtojek commented Nov 24, 2021

jsoriano commented Nov 24, 2021

fearful-symmetry commented Nov 24, 2021 • edited Loading

mtojek commented Nov 24, 2021

fearful-symmetry commented Nov 29, 2021

mtojek commented Nov 30, 2021

fearful-symmetry commented Nov 30, 2021

mtojek commented Nov 30, 2021 • edited Loading

fearful-symmetry commented Nov 30, 2021

mtojek commented Nov 30, 2021 • edited Loading

jsoriano commented Dec 1, 2021 • edited Loading

fearful-symmetry commented Dec 1, 2021

mtojek commented Dec 2, 2021

jsoriano commented Dec 2, 2021

mtojek commented Dec 2, 2021 • edited Loading

mtojek commented Apr 1, 2022

elasticmachine commented Nov 24, 2021 •

edited

Loading

mtojek Nov 24, 2021 •

edited

Loading

fearful-symmetry commented Nov 24, 2021 •

edited

Loading

mtojek commented Nov 30, 2021 •

edited

Loading

mtojek commented Nov 30, 2021 •

edited

Loading

jsoriano commented Dec 1, 2021 •

edited

Loading

mtojek commented Dec 2, 2021 •

edited

Loading