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

Get-WmiObject - Invalid class "Win32_Service" - Workaround #33322

Merged
merged 12 commits into from
Nov 3, 2022

Conversation

stryngs
Copy link
Contributor

@stryngs stryngs commented Oct 12, 2022

What does this PR do?

This PR adds a workaround to a situation in Windows where calling Get-WmiObject returns "Win32_Service" as an Invalid class.

Why is it important?

This is important to users which remotely deploy and install any of the Beats in a Windows environment. Rather than solving the class issue I focused on a workaround which can be provided until a proper solution to the class issue is found.

Checklist

  • My code follows the style guidelines of this project
    - [ ] I have commented my code, particularly in hard-to-understand areas
    - [ ] I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
    - [ ] I have added tests that prove my fix is effective or that my feature works
    - [ ] I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • N/A

How to test this PR locally

Build any Beats package and then run the associated installer for Windows.

Related issues

  • N/A

Use cases

Windows zipfile deployments for service installations.

Screenshots

N/A

Logs

N/A

@stryngs stryngs requested a review from a team as a code owner October 12, 2022 02:16
@stryngs stryngs requested review from cmacknz and leehinman and removed request for a team October 12, 2022 02:16
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 12, 2022
@cla-checker-service
Copy link

cla-checker-service bot commented Oct 12, 2022

💚 CLA has been signed

@mergify
Copy link
Contributor

mergify bot commented Oct 12, 2022

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @stryngs? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@stryngs
Copy link
Contributor Author

stryngs commented Oct 12, 2022

x Author of the following commits did not sign a Contributor Agreement: 257f588, a5eae20

Please, read and sign the above mentioned agreement if you want to contribute to this project

Signed.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 12, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-11-03T12:52:40.010+0000

  • Duration: 111 min 22 sec

Test stats 🧪

Test Results
Failed 0
Passed 23739
Skipped 1951
Total 25690

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@cmacknz cmacknz added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Oct 12, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 12, 2022
@cmacknz
Copy link
Member

cmacknz commented Oct 12, 2022

/test

@stryngs
Copy link
Contributor Author

stryngs commented Oct 18, 2022

What do I need to do to keep this moving forward?

@cmacknz
Copy link
Member

cmacknz commented Oct 18, 2022

@leehinman is looking at this. The main concern is making sure this change will work as expected across the various Windows versions we need to support.

@stryngs
Copy link
Contributor Author

stryngs commented Oct 19, 2022

Awesome and glad to hear it.

Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

Will also need a changelog entry.

$service.StopService()
Start-Sleep -s 1
$service.delete()
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets not do the try/catch and just change to the code in the catch block.

I tested the code in the catch block on Windows 11, Windows Server 2012, 2016 & 2019. Works fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the requested changes, please let me know if there is anything further needed. Thanks for moving so quick on this!

@stryngs
Copy link
Contributor Author

stryngs commented Nov 2, 2022

What else needs to be done for this?

@mergify
Copy link
Contributor

mergify bot commented Nov 2, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b main upstream/main
git merge upstream/main
git push upstream main

Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

LGTM

@cmacknz
Copy link
Member

cmacknz commented Nov 2, 2022

We just need the merge conflict in the changelog resolved and we can merge this.

@stryngs
Copy link
Contributor Author

stryngs commented Nov 2, 2022

I'm not the best with git. I tried the steps and got this:

~/beats$ git fetch upstream
fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

What can I do to fix?

@cmacknz
Copy link
Member

cmacknz commented Nov 2, 2022

I was able to resolve the conflict on your branch myself via the GitHub UI. The command above is assuming you set the elastic/beats repository as a remote named upstream. You can run git remote -v to see which remote name references the elastic/beats repository instead of your fork of it.

@mergify
Copy link
Contributor

mergify bot commented Nov 3, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b main upstream/main
git merge upstream/main
git push upstream main

@cmacknz cmacknz added v8.6.0 QA:Needs Validation Needs validation by the QA Team labels Nov 3, 2022
@cmacknz cmacknz merged commit 4e58e31 into elastic:main Nov 3, 2022
@cmacknz
Copy link
Member

cmacknz commented Nov 3, 2022

Merged! This will be in the 8.6 release for Beats.

@stryngs
Copy link
Contributor Author

stryngs commented Mar 13, 2023

I'm looking at 8.6.2 and still do not see this pulled in?

@cmacknz
Copy link
Member

cmacknz commented Mar 21, 2023

It should be in 8.6.2, Git certainly thinks it is looking at the tags on the merged commit 4e58e31

@stryngs
Copy link
Contributor Author

stryngs commented May 5, 2023

We have 8.6.2 on site and I need to apply this change by hand. Maybe someone didn't push a specific button or?

@cmacknz
Copy link
Member

cmacknz commented May 5, 2023

The commit is part of the 8.6.2 release, Git shows that it is and you can see it if you go far enough back in the 8.6.2 commit history. It is one of the last commits in https://github.com/elastic/beats/commits/v8.6.2?after=9b77c2c135c228c2eedc310f6e975bb1a76169b1+139&branch=v8.6.2&qualified_name=refs%2Ftags%2Fv8.6.2

If it isn't working it is possible something was missed as part of the fix here, possibly something that is part of the build and packaging step.

chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
* Get-WmiObject - Invalid class "Win32_Service" - Workaround

* try/catch replace + changelog documentation

* Changelog fix

Co-authored-by: Lee E. Hinman <[email protected]>
Co-authored-by: Craig MacKenzie <[email protected]>
@BrunoMacias
Copy link

Was there more information about the root cause of win32_service issues?

@stryngs
Copy link
Contributor Author

stryngs commented Mar 22, 2024

The commit is part of the 8.6.2 release, Git shows that it is and you can see it if you go far enough back in the 8.6.2 commit history. It is one of the last commits in https://github.com/elastic/beats/commits/v8.6.2?after=9b77c2c135c228c2eedc310f6e975bb1a76169b1+139&branch=v8.6.2&qualified_name=refs%2Ftags%2Fv8.6.2

If it isn't working it is possible something was missed as part of the fix here, possibly something that is part of the build and packaging step.

This never made it into the release. I am currently working at 8.11.x and not seeing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA:Needs Validation Needs validation by the QA Team Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants