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

Rebase plugin from zip patch #235

Merged
merged 4 commits into from
Feb 3, 2021
Merged

Rebase plugin from zip patch #235

merged 4 commits into from
Feb 3, 2021

Conversation

XMol
Copy link

@XMol XMol commented Jan 5, 2021

Pull Request (PR) description

This Pull Request is a continuation of #217, which was in need of a rebase onto master since Aug 2020.

This Pull Request (PR) fixes the following issues

Fixes #173
Closes #217

@XMol
Copy link
Author

XMol commented Jan 5, 2021

@bastelfreak, may we ask you for assistance?

I've tried to fix the problem with the 'create plugin resource with url' unit test (spec/acceptance/grafana_plugin_spec.rb) by installing a proper plugin. As far as my local tests with Grafana have shown, the minimal manifest achieves the right thing and the plugin should be installed (confirmed by actions report). However, the shell command listing all installed Grafana plugins does not include it. Instead, only 'grafana-simple-json-datasource' is shown, which is probably installed by previous unit tests. Before this PR I've never had to look into Ruby unit tests, so I'm clueless what could have gone wrong where. In case the problem roots with the Docker images and neither the Puppet code or Ruby unit tests, then I possibly ask for too much of your time analyzing it for us - my appoligies.

Best regards,
Xavier.

/cc @apetzo

@bastelfreak
Copy link
Member

hey @XMol . can you rebase against our current master branch? The history in the PR currently contains many commits that are already merged, that makes it hard to read/debug. If you need any help please join our IRC channel #voxpupuli on freenode or #voxpupuli on http://slack.puppet.com/

@bastelfreak bastelfreak added needs-rebase needs-work not ready to merge just yet labels Jan 5, 2021
@XMol
Copy link
Author

XMol commented Jan 7, 2021

hey @XMol . can you rebase against our current master branch?

I thought I did that, but I'll try again...

@XMol
Copy link
Author

XMol commented Jan 7, 2021

Now I see what I've done wrong. Instead of rebasing the patch branch onto master I rebased master onto it. Now I'll redo that without setting myself as committer, which should hopefully reduce the comparison overview to just the actual new commits.

Sorry for all the noise.

@XMol
Copy link
Author

XMol commented Jan 7, 2021

And lastly, I should rebase onto the master branch for the repository we set up the pull request for (instead of bfraser/grafana). Now the history/comparison looks sensible.

@bastelfreak
Copy link
Member

thanks for the rebase! Can you take a look at the failing tests?

@XMol
Copy link
Author

XMol commented Jan 7, 2021

Hi @bastelfreak,

that's the thing we asked you for help on previously. The error is still the same: Even though the Grafana plugin was successfully installed according to the output of the applied temporary Puppet manifest, grafana-cli plugins ls only lists 'grafana-simple-json-datasource', which I assume to be a remnant of earlier unit tests. At least, that's how I interpret the report for the acceptance spec test... This problem does not come up with my own tests for this patch, thus I don't know whether the problem is with the definition of the tests (as part of this project) or the Docker images.

@XMol XMol changed the title Rebase plugin from zip patch WIP: Rebase plugin from zip patch Jan 11, 2021
@XMol
Copy link
Author

XMol commented Jan 11, 2021

I've tried to increase the verbosity of the whole procedure to possibly identify what went wrong, but failed to find an explanation for why the installed plugin is not listed.

Snippet from latest CentOS 6 actions log lines 7301..7305:

Debug: Prefetching grafana_cli resources for grafana_plugin
Debug: Executing: '/usr/sbin/grafana-cli plugins ls'
Debug: Found grafana plugin grafana-simple-json-datasource 1.4.1
Debug: Executing: '/usr/sbin/grafana-cli --pluginUrl https://github.com/grafana/worldmap-panel/releases/download/v0.3.2/grafana-worldmap-panel-0.3.2.zip --debug plugins install grafana-worldmap-panel'
Notice: /Stage[main]/Main/Grafana_plugin[grafana-worldmap-panel]/ensure: created

So "grafana-simple-json-datasource" is found to be installed already (by previous tests) and "grafana-worldmap-panel" is supposedly installed successfully this time. However, the verification for that fails...

Failure/Error: expect(r.stdout).to match(%r{grafana-worldmap-panel})
  expected "installed plugins:\ngrafana-simple-json-datasource @ 1.4.1 \n\nRestart grafana after installing plugins . <service grafana-server restart>\n\n" to match /grafana-worldmap-panel/
  Diff:
    @@ -1,4 +1,7 @@
    -/grafana-worldmap-panel/
    +installed plugins:
    +grafana-simple-json-datasource @ 1.4.1 
    +
    +Restart grafana after installing plugins . <service grafana-server restart>

I've double-checked in local tests that...

  • grafana-cli plugins install fails when the plugin wasn't actually installed because e.g. the user has not sufficient rights to write to the plugins directory or the provided URL is fake.
  • grafana-cli plugins ls ought to list the properly installed plugins from the default plugins directory, even when plugins are installed by URL.
  • puppet apply -e 'grafana_plugin { "grafana-worldmap-panel": ensure => "present", plugin_url => "https://github.com/grafana/worldmap-panel/releases/download/v0.3.2/grafana-worldmap-panel-0.3.2.zip" }' successfully installs the "grafana-worldmap-panel" plugin, as confirmed by grafana-cli plugins ls`.
  • puppet apply -e 'grafana_plugin { "grafana-example-custom-plugin": ensure => "present", plugin_url => "https://github.com/example/example-custom-plugin/zipball/v1.0.0" }'
    fails, even though it succeeded for the original PR

At this point, I'm out of ideas on how I could debug this any further or what could be the underlying issue, so I again formally ask you, @bastelfreak, for assistance.

@bastelfreak
Copy link
Member

I've no idea why it fails :( Maybe one of the others @voxpupuli/collaborators has some more knowledge about Grafana

@lucywyman
Copy link

Is there a path Grafana plugins get installed to you could check? I'm not sure if the plugin is installed and the grafana-cli just isn't seeing it (I see the message it needs a refresh after installing plugins), or if it isn't getting downloaded at all.

@XMol
Copy link
Author

XMol commented Jan 20, 2021

Hi Lucy, thanks for the idea, there is indeed a path we could just look into. The notice about the failed reload is not critical. The Docker image has no Grafana service prepared and for the installation of the plugin a reload of the service is not strictly necessary (but, as is evident, Puppet would reload the service under normal circumstances). I'll add an ls with the next commit.

@XMol
Copy link
Author

XMol commented Jan 20, 2021

So, ls on the plugins directory confirms that no plugin was installed. Next I'll try and install the plugin without Puppet and see whether that also fails. If so, then I'm even more clueless than before... 😭

@wyardley
Copy link

@XMol My only suggestion is get the integration test running locally. Running it with BEAKER_destroy set to no will make this a lot easier to test out (you can ssh or exec in and run manifests or play with commands)

@XMol
Copy link
Author

XMol commented Jan 20, 2021

Hi Will,

I'm sorry, I don't know how to do that. I've never worked with Beaker before and neither do I have any experience with Git actions.


So installing the plugin by shell command works as expected. Then I have one more idea: Maybe puppet isn't run with root privileges? Then the plugin is installed in a different plugin directory then what grafana-cli would look into when called in shell?

@XMol
Copy link
Author

XMol commented Jan 20, 2021

Hmm, no, my idea must be wrong. grafana-cli would install in the same default plugins directory (/var/lib/grafana/plugins), even when not executed as root. Only grafana-cli plugins ls won't look into that same default plugins directory without root id.

@XMol
Copy link
Author

XMol commented Jan 20, 2021

With the last two commits, it seems clear that there is a difference between running grafana-cli plugins install from shell directly or through Puppet. Even when just applying the Puppet grafana_plugin resource, which literally runs the same shell command, the plugin is not found in the plugins directory.

I say "same shell command", but that strictly isn't true. Puppet calls /usr/sbin/grafana-cli, while grafana-cli in shell might find /sbin/grafana-cli first. Though, at least on my installation, both files are identical (compared by md5sum), hence I don't think that is a factor.

Does someone else have any more ideas? I'd pick up on Will's suggestion, if I knew how to do that - if you can point me to a tutorial for such situations, then I could follow it up once I've learned the ropes. But still, that would only speed up the debugging, while I'm lost on what to debug next...

@reidmv
Copy link
Member

reidmv commented Jan 20, 2021

@XMol I'm coming into this blind with just a debugging idea.

Usually, when there is a difference between running a command on the CLI and Puppet running the command, it turns out to be related to environment variables. Puppet exec resources strip ALL environment variables out, including some that are important to some CLI utilities, such as HOME and USER. You might be able to check for this by trying to see if running the command in a sanitized environment results in the same thing Puppet sees when it runs it. E.g.

env -i /usr/sbin/grafana-cli plugins install

@XMol
Copy link
Author

XMol commented Jan 21, 2021

Thank you for the hint, Reid, I'll check on that soon. On the advertise Puppet Slack #voxpupuli channel I also got a lot of help from Mike on how to debug the Docker containers, without rerunning actions on GitHub every time. That's a bit more complicated, so will have to spend more time on that.

@XMol
Copy link
Author

XMol commented Jan 21, 2021

Following Reid's hint:

[root@grafana-test ~]# env -i /usr/sbin/grafana-cli plugins ls | grep worldmap

[root@grafana-test ~]# env -i /usr/sbin/grafana-cli --pluginUrl="https://github.com/grafana/worldmap-panel/releases/download/v0.3.2/grafana-worldmap-panel-0.3.2.zip" plugins install grafana-worldmap-panel
installing grafana-worldmap-panel @
from: https://github.com/grafana/worldmap-panel/releases/download/v0.3.2/grafana-worldmap-panel-0.3.2.zip
into: /var/lib/grafana/plugins

✔ Installed grafana-worldmap-panel successfully

Restart grafana after installing plugins . <service grafana-server restart>

[root@grafana-test ~]# env -i /usr/sbin/grafana-cli plugins ls | grep worldmap
grafana-worldmap-panel @ 0.3.2

Looks like the grafana-cli does not depend on any environment settings.

@XMol
Copy link
Author

XMol commented Jan 25, 2021

To keep everyone posted, thanks to the very kind help of @crazymind1337, I now have a functional setup to reproduce the same errors as during the GitHub actions cycle locally. Indeed, installing the plugin with the grafana-cli works fine, while is does not through Puppet.

@crazymind1337
Copy link
Member

We found out: The problem of the failing test is not the puppet code. It is the way the tests are applied. I didn't know that every test is applied to the same docker container, but it is happening. Within spec/acceptance/class_spec.rb there is a test context 'beta release' do which installs a beta repo and 6.0.0-beta3 version of grafana. Every test coming afterwords will be executed with this beta version of grafana installed. I am really wondering why those tests did not explode earlier. I think the accpetance test do need some bigger refactoring. Especially we should ensure the correct versions for the tests.

@bastelfreak
Copy link
Member

hey all. thanks for the awesome work! Is this still WIP or can we review and merge it?

@XMol
Copy link
Author

XMol commented Feb 2, 2021

Alright, I'm sorry that I could not continue this work earlier, but here it finally is.

Thanks to the extensive assisstance by @crazymind1337, the issue was found to not be with the additional Puppet code, but with the acceptance tests. In particular, that test which downgraded from the latest stable Grafana version onto 6.0.0-beta3 (specifically). As it turns out, installing a plugin by HTTP-url is broken for this Grafana version. So what I now have done with this pull request:

  • Rebase once more onto the current master branch (which now includes PR Fix types to work with 'puppet generate types' #236)
  • Squashed all commits of @jwbraucher as the original author into a single commit
  • Replaced the dummy plugin that ought to be installed in the acceptance test with grafana-simple-json-datasource.
    • Note that this plugin is installed once with standard procedure, so it is uninstalled using the grafana-cli and then installed again via Puppet and HTTP-url.
  • The test that migrated the Grafana version to the beta branch is reverted with another, new test which only reverts back to stable.
    • In my humble opinion, this is not an elegant solution. But as @crazymind1337 already hinted at, a major revision of the tests might be in order, which is way beyond my current capabilities as well as the scope of this PR,
    • Also, the previous test about the beta release has been adapted to install the latest available beta release (instead of 6.0.0-beta3 explicitly).

Hopefully this will met the expectations voxpupuli has towards PRs, while the work previously invested by @jwbraucher is honored. If so, yes @bastelfreak, you may accept it now. 🙂

Best regards,
Xavier.

/cc @apetzo

@XMol XMol changed the title WIP: Rebase plugin from zip patch Rebase plugin from zip patch Feb 2, 2021
Copy link
Member

@bastelfreak bastelfreak left a comment

Choose a reason for hiding this comment

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

looks perfect to me. I will keep this open for a few days in case someone else want's to review it as well.

Dockerfile Outdated Show resolved Hide resolved
lib/puppet/provider/grafana_plugin/grafana_cli.rb Outdated Show resolved Hide resolved
lib/puppet/type/grafana_plugin.rb Outdated Show resolved Hide resolved
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Overall 👍

README.md Outdated Show resolved Hide resolved
spec/acceptance/class_spec.rb Outdated Show resolved Hide resolved
spec/acceptance/class_spec.rb Show resolved Hide resolved
@crazymind1337
Copy link
Member

If there a no further objections I will merge this.

@crazymind1337 crazymind1337 merged commit eece7d7 into voxpupuli:master Feb 3, 2021
@crazymind1337 crazymind1337 removed needs-rebase needs-work not ready to merge just yet labels Feb 3, 2021
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.

Feature Request: Support for Grafana Plugin install by URL
8 participants