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

feat: Implement Device System Events #4101

Merged
merged 8 commits into from
Aug 2, 2022

Conversation

lenny-goodell
Copy link
Member

@lenny-goodell lenny-goodell commented Jul 21, 2022

Also, refactored to use common Messaging Bootstrap Handler

closes #4099

Implements related ADR

Dependent on the following PRs:

Signed-off-by: Leonard Goodell [email protected]

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-go/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

  • I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
  • I am not introducing a new dependency (add notes below if you are)
  • I have added unit tests for the new feature or bug fix (if not, why?)
  • I have fully tested (add details below) this the new feature or bug fix (if not, why?)
  • I have opened a PR for the related docs change (if not, why?)
    feat: Document Core Metadata Device System Events edgex-docs#830

Testing Instructions

Run edgex stack with Device Virtual
Stop Core Metadata container
Build and run this branch locally (require branches from PR referenced above) with DEBUG enabled
Using curl or postman delete, add and remove Device Virtual devices (can't use EdgeX UI from docker since core-metadata is running locally)
Verify log show debug message that System Event was published

New Dependency Instructions (If applicable)

internal/core/metadata/application/device.go Outdated Show resolved Hide resolved
internal/core/metadata/application/device.go Outdated Show resolved Hide resolved
internal/core/metadata/application/device.go Outdated Show resolved Hide resolved
@lenny-goodell lenny-goodell marked this pull request as ready for review July 25, 2022 20:33
@lenny-goodell lenny-goodell requested a review from cloudxxx8 July 25, 2022 21:36
internal/core/metadata/application/device.go Outdated Show resolved Hide resolved
internal/core/metadata/application/device.go Outdated Show resolved Hide resolved
internal/core/metadata/application/device.go Outdated Show resolved Hide resolved
internal/core/metadata/application/device.go Outdated Show resolved Hide resolved
@lenny-goodell lenny-goodell force-pushed the device-system-events branch from d602e86 to 10c8b50 Compare July 27, 2022 16:31
@lenny-goodell lenny-goodell requested a review from cloudxxx8 July 27, 2022 16:32
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2022

Codecov Report

Merging #4101 (d69e356) into main (062af8d) will decrease coverage by 3.69%.
The diff coverage is 75.86%.

@@            Coverage Diff             @@
##             main    #4101      +/-   ##
==========================================
- Coverage   47.32%   43.63%   -3.70%     
==========================================
  Files         114      120       +6     
  Lines        9850    10591     +741     
==========================================
- Hits         4662     4621      -41     
- Misses       4801     5586     +785     
+ Partials      387      384       -3     
Impacted Files Coverage Δ
internal/core/metadata/application/device.go 13.09% <75.86%> (ø)
...rnal/core/metadata/application/provisionwatcher.go 0.00% <0.00%> (ø)
...ternal/core/metadata/application/deviceresource.go 0.00% <0.00%> (ø)
...nternal/core/metadata/application/devicecommand.go 0.00% <0.00%> (ø)
...nternal/core/metadata/application/deviceprofile.go 0.00% <0.00%> (ø)
internal/core/metadata/application/notify.go 0.00% <0.00%> (ø)
...nternal/core/metadata/application/deviceservice.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 062af8d...d69e356. Read the comment docs.

@lenny-goodell lenny-goodell force-pushed the device-system-events branch from c91a622 to ac32090 Compare July 27, 2022 23:21
cloudxxx8
cloudxxx8 previously approved these changes Jul 27, 2022
Copy link
Member

@cloudxxx8 cloudxxx8 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

The snap fails to start because security-secretstore-setup exits with non-zero code. The CI logs currently don't get the logs when the snap fails to even start (we are working on improving this).

I downloaded the build from Github (see for example here; scroll down) and installed manually to check the logs:

$ snap install --dangerous ./edgexfoundry_2.3.0-dev.30_amd64.snap
Run service command "start" for services ["consul" "core-command" "core-data" "core-metadata" "kong-daemon" "postgres" "redis" "security-bootstrapper-redis" "security-consul-bootstrapper" "security-proxy-setup" "security-secretstore-setup" "vault"] of snap "edgexfoundry" (systemctl command [start snap.edgexfoundry.security-secretstore-setup.service] failed with exit status 1: Job for snap.edgexfoundry.security-secretstore-setup.service failed because the control process exited with error code.

$ sudo journalctl -n 1000 | grep edgexfoundry | grep ERROR
level=ERROR ts=2022-07-28T08:14:23.57437856Z app=security-secretstore-setup source=init.go:329 msg="token provider failed: /snap/edgexfoundry/x1/bin/security-file-token-provider terminated with non-zero exit code 1"

Unfortunately, security-secretstore-setup is not printing the error message. Unrelated to this PR, it looks like standard output and standard error aren't captured by EdgeX logger here. This code sets to use os output streams but not sure if this is ever called.

I could not figure out why the changes in this PR are causing this failure.

Full logs: edgexfoundry_2.3.0-dev.30.logs.txt, see line 493

@lenny-goodell
Copy link
Member Author

@farshidtz , @jim-wang-intel create this PR to test which go-mod and which version has triggered this. We have narrowed it down to go-mod-bootstrap v2.2.1-dev.9
#4105

I am taking a look at the commit for that to see how it could be impacting this.

@lenny-goodell
Copy link
Member Author

@farshidtz , that change to go-mod-bootstrap was to make sure the exit code it set properly when bootstrapping fails. Before it always returned with 0 exit code.

I suspect that the issue with Secret Store setup in the test was always there, be now has the proper exit code.

@lenny-goodell
Copy link
Member Author

@farshidtz , found the culprit !

The file token provider is incorrectly returning false in its bootstrap hander which now causes it to exit with non-zero exit code.

We'll get a fix in for that which will un-block this PR.

@lenny-goodell lenny-goodell force-pushed the device-system-events branch from d69e356 to d98ec5e Compare July 29, 2022 17:07
@lenny-goodell
Copy link
Member Author

@jim-wang-intel , @farshidtz , Rebased this PR to pull in fixes from the other PR.

@lenny-goodell lenny-goodell marked this pull request as draft July 29, 2022 17:11
@lenny-goodell
Copy link
Member Author

lenny-goodell commented Jul 29, 2022

Put in draft to it doesn't get merged until this PR is merged otherwise TAF tests will fail.
edgexfoundry/edgex-compose#261

Leonard Goodell added 5 commits August 2, 2022 10:23
Also, refactor to use common Messaging Bootstrap Handler

closes edgexfoundry#4099

Signed-off-by: Leonard Goodell <[email protected]>
Signed-off-by: Leonard Goodell <[email protected]>
Leonard Goodell added 2 commits August 2, 2022 10:23
@lenny-goodell lenny-goodell force-pushed the device-system-events branch 2 times, most recently from ed9310a to d0dd691 Compare August 2, 2022 16:30
@lenny-goodell lenny-goodell marked this pull request as ready for review August 2, 2022 16:31
Newest version may be causing issue and not needed in this PR.

Signed-off-by: Leonard Goodell <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.5% 0.5% Duplication

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.

Core Metadata publish Device System Events
5 participants