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

MG-2354 - Fix Update Bootstrap Config State #2356

Merged
merged 9 commits into from
Jul 25, 2024

Conversation

nyagamunene
Copy link
Contributor

What type of PR is this?

This is a bug fix because it fixes the following issue: #2354

What does this do?

It changes how the ConnectThing and DisconnectThing in repo makes the query. It ensure that not all rows are updated

Which issue(s) does this PR fix/relate to?

Have you included tests for your changes?

Yes, I have included tests for my changes.

Did you document any new/modified feature?

N/A

Notes

@nyagamunene nyagamunene self-assigned this Jul 23, 2024
@nyagamunene nyagamunene marked this pull request as ready for review July 24, 2024 11:28
@JeffMboya
Copy link
Contributor

@nyagamunene why is method changeState removed?

@dborovcanin
Copy link
Collaborator

Changing state does not make much sense anymore from the Bootstrap perspective. I.e. we have a manual option to change Things' state now, and the state has nothing to do with BS configuration. @arvindh123 Please review.

@nyagamunene
Copy link
Contributor Author

@nyagamunene why is method changeState removed?

The reason was that the ConnectThingHandler and the DisconnectThingHandler methods consume events from things and when there is a change it updates the repo accordingly hence no need for changeState which makes it redundant.

Comment on lines 148 to 164

// Check for existing connection between channel and thing
// If it exists set state to acive
state := Inactive
pm := mgsdk.PageMetadata{}
for _, channel := range cfg.Channels {
tp, err := bs.sdk.ThingsByChannel(channel.ID, pm, token)
if err != nil {
return Config{}, errors.Wrap(svcerr.ErrMalformedEntity, err)
}
for _, thing := range tp.Things {
if thing.ID == cfg.ThingID {
state = Active
break
}
}
}
Copy link
Contributor

@JeffMboya JeffMboya Jul 24, 2024

Choose a reason for hiding this comment

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

@nyagamunene this will be resolve through a separate endpoint here: #2301. @arvindh123 please confirm

Signed-off-by: nyagamunene <[email protected]>
@arvindh123
Copy link
Contributor

From the below logs we can see on bootstrap state change , bootstrap service sends connect/disconnect from channel to things service.
Then things service emit the connect/disconnect event, which is again consumed by bootstrap service

{"time":"2024-07-25T09:21:48.15854206Z","level":"INFO","msg":"Disconnect thing handler completed successfully","duration":"8.397523ms","channel_id":"6edf5552-dd76-4d9c-ad4f-f62481ad4c58","thing_id":"7af3a95c-a1fa-4379-8f0b-fc7e8a5af3a3"}
{"time":"2024-07-25T09:21:48.182582857Z","level":"INFO","msg":"Change thing state completed successfully","duration":"130.098491ms","id":"7af3a95c-a1fa-4379-8f0b-fc7e8a5af3a3","state":0}

The problem happen exactly at Disconnect and Connect handlers in repo where there is wrong query

UPDATE configs SET state = $1 WHERE EXISTS ( SELECT 1 FROM connections WHERE config_id = $2 AND channel_id = $3)

This query always update all the columns in bootstrap service.

Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Copy link
Contributor

@arvindh123 arvindh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@dborovcanin dborovcanin merged commit c3e7159 into absmach:main Jul 25, 2024
5 of 6 checks passed
andychao217 pushed a commit to andychao217/magistrala that referenced this pull request Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Bug: Update Bootstrap Config State Updates all Configs States
5 participants