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

add subscription http endpoints #23

Merged
merged 6 commits into from
Aug 20, 2020
Merged

Conversation

mikemrm
Copy link
Contributor

@mikemrm mikemrm commented Aug 11, 2020

Description

Adds http endpoints to list or get subscribers. This allows for tooling to detect whether a server is currently connected to hegel to determine how it should proceed.

Example Output

$ curl -s localhost:50061/subscriptions | jq '.'
{
  "abcdefab-cdef-abcd-efab-abcdefabcdef": {
    "id": "abcdefab-cdef-abcd-efab-abcdefabcdef",
    "ip": "127.0.0.1",
    "init_duration": 2000004,
    "started_at": "2020-08-11T20:55:04.56156941Z"
  }
}

$ curl -s localhost:50061/subscriptions/abcdefab-cdef-abcd-efab-abcdefabcdef | jq '.'
{
  "id": "abcdefab-cdef-abcd-efab-abcdefabcdef",
  "ip": "127.0.0.1",
  "init_duration": 2000004,
  "started_at": "2020-08-11T20:55:04.56156941Z"
}

$ curl -s localhost:50061/subscriptions/abcdefab-cdef-abcd-efab-abcdefabcdee | jq '.'
{
  "error": {
    "comment": "item not found",
    "error": "abcdefab-cdef-abcd-efab-abcdefabcdee not found"
  }
}

@mikemrm mikemrm force-pushed the ENG-7045 branch 5 times, most recently from 5605e26 to c6c33cf Compare August 12, 2020 16:08
@mikemrm mikemrm requested a review from a team August 12, 2020 20:50
@mikemrm mikemrm force-pushed the ENG-7045 branch 7 times, most recently from 3d452c5 to 603f27e Compare August 18, 2020 03:38
@mikemrm
Copy link
Contributor Author

mikemrm commented Aug 18, 2020

@kdeng3849 tagging you on here for visibility. I've had to make quite a few changes in order to make hegel backwards compatible with osie/osie preinstall. Issues were primarily with metadata instance, and storage attributes.

@mikemrm mikemrm force-pushed the ENG-7045 branch 2 times, most recently from e101816 to 00eacac Compare August 18, 2020 17:40
@mikemrm mikemrm requested a review from mmlb August 18, 2020 17:41
Copy link
Contributor

@mmlb mmlb left a comment

Choose a reason for hiding this comment

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

This commit message is actually missing the most important part, that the suffix is very important to tools we run that uses this data, so needs to stay (sgdisk)

main.go Outdated Show resolved Hide resolved
grpc_server.go Outdated Show resolved Hide resolved
grpc_server.go Outdated
defer func() {
s.subLock.Lock()
defer s.subLock.Unlock()
if cSub, ok := s.subscriptions[id]; ok && cSub.updateChan == sub.updateChan {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need the ok check I think since for the == to be true then ok will also be true. If ok is false, there's no way cSub.updateChan == sub.updateChan because cSub.updateChan will be nil and sub.updateChan is never nil at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also deserves some sort of comment that this is a guard against deleting a sub that has already been deleted because of a reconnection attempt or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cSub could be null if the id doesn't already exist. But will add a comment

Copy link
Contributor

Choose a reason for hiding this comment

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

right, but sub isn't null so they don't == so you wont delete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on a normal disconnect, cSub and sub could be equal. As another connection would not have come in and overwrite the s.subscriptions[id]

http_handlers.go Outdated Show resolved Hide resolved
http_handlers.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
grpc_server.go Outdated Show resolved Hide resolved
grpc_server_test.go Outdated Show resolved Hide resolved
upgrade pkg/env

Signed-off-by: Mike Mason <[email protected]>
@mikemrm mikemrm force-pushed the ENG-7045 branch 2 times, most recently from 85f7cf2 to 0acef54 Compare August 20, 2020 00:55
@mikemrm mikemrm requested a review from mmlb August 20, 2020 01:09
@mmlb
Copy link
Contributor

mmlb commented Aug 20, 2020

This commit message is actually missing the most important part, that the suffix is very important to tools we run that uses this data, so needs to stay (sgdisk)

@mikemrm ^

@mikemrm
Copy link
Contributor Author

mikemrm commented Aug 20, 2020

Sorry, missed that. I've added more details about the sgdisk in the comment.

sgdisk is the tool used in osie to build partitions. sgdisk
supports multiple methods to define the size of those partitions
and therefore this value should not be modified.

Signed-off-by: Mike Mason <[email protected]>
@mikemrm mikemrm merged commit a3138d4 into tinkerbell:master Aug 20, 2020
@mikemrm mikemrm deleted the ENG-7045 branch August 20, 2020 16:38
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.

2 participants