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 NSM topology probe #1636

Merged
merged 24 commits into from May 17, 2019
Merged

Conversation

@matrohon
Copy link
Contributor

@matrohon matrohon commented Jan 30, 2019

This patch contains the probe used for NSM demos.
Rendering patches, related to javascript files, will be part of subsequent patches.

topology/probes/nsm/nsm.go Outdated Show resolved Hide resolved
topology/probes/nsm/nsm.go Show resolved Hide resolved

// Stop ....
func (p *Probe) Stop() {
atomic.CompareAndSwapInt64(&p.state, common.RunningState, common.StoppingState)
Copy link
Member

@lebauce lebauce Jan 30, 2019

Choose a reason for hiding this comment

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

You should test the return value of CompareAndSwapInt64:

if !atomic.CompareAndSwapInt64(&p.state, common.RunningState, common.StoppingState) {
    return
}

Copy link
Contributor Author

@matrohon matrohon Jan 31, 2019

Choose a reason for hiding this comment

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

What is going to happen the swap fails? subsequent Stop are sent by the analyzer?

Copy link
Member

@lebauce lebauce Jan 31, 2019

Choose a reason for hiding this comment

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

No. But this means the probe was not started.

go p.monitorCrossConnects(mgr.Status.URL)
}
}
for atomic.LoadInt64(&p.state) == common.RunningState {
Copy link
Member

@lebauce lebauce Jan 30, 2019

Choose a reason for hiding this comment

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

This waiting loop doesn't seem to be useful. Why not simply let the function exit ? (I don't think you need to execute run in a goroutine)

Copy link
Contributor Author

@matrohon matrohon Jan 31, 2019

Choose a reason for hiding this comment

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

you're right, let's remove run() and move this code to Start()

topology/probes/nsm/connection.go Outdated Show resolved Hide resolved
topology/probes/nsm/connection.go Show resolved Hide resolved
topology/probes/nsm/connection.go Show resolved Hide resolved
topology/probes/nsm/connection.go Outdated Show resolved Hide resolved
topology/probes/nsm/nsm.go Outdated Show resolved Hide resolved
topology/probes/nsm/nsm.go Outdated Show resolved Hide resolved
@matrohon matrohon force-pushed the nsm-probe-only branch 3 times, most recently from 481fbed to 4f3dea2 Jan 31, 2019
Copy link
Contributor Author

@matrohon matrohon left a comment

Thanks for the review @lebauce, i've addressed your comments in 4f3dea2


// Stop ....
func (p *Probe) Stop() {
atomic.CompareAndSwapInt64(&p.state, common.RunningState, common.StoppingState)
Copy link
Contributor Author

@matrohon matrohon Jan 31, 2019

Choose a reason for hiding this comment

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

What is going to happen the swap fails? subsequent Stop are sent by the analyzer?

go p.monitorCrossConnects(mgr.Status.URL)
}
}
for atomic.LoadInt64(&p.state) == common.RunningState {
Copy link
Contributor Author

@matrohon matrohon Jan 31, 2019

Choose a reason for hiding this comment

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

you're right, let's remove run() and move this code to Start()

topology/probes/nsm/nsm.go Outdated Show resolved Hide resolved
}

// OnNodeAdded event
// We assume skydive has locked the graph before calling this function
Copy link
Member

@lebauce lebauce Jan 31, 2019

Choose a reason for hiding this comment

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

nit: correct, therefor this comment can safely be removed :-)

Copy link
Contributor Author

@matrohon matrohon Jan 31, 2019

Choose a reason for hiding this comment

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

fine, that was just a reminder :)

topology/probes/nsm/nsm.go Outdated Show resolved Hide resolved
@lebauce
Copy link
Member

@lebauce lebauce commented Feb 6, 2019

run skydive-functional-tests-backend-orientdb

@matrohon matrohon force-pushed the nsm-probe-only branch 2 times, most recently from a121158 to de1d1f9 Feb 26, 2019
@matrohon
Copy link
Contributor Author

@matrohon matrohon commented Feb 27, 2019

run skydive-functional-tests-backend-orientdb

@hunchback
Copy link
Collaborator

@hunchback hunchback commented Mar 1, 2019

You should add skydive CI tests to maintain probe quality if not handled in this PR then open an issue to track.

This would include:

  • adding a new job skydive-nsm-tests in scripts/ci/jobs
  • adding a script to setup nsm on a ci host: scripts/ci/install-nsm.sh
  • adding a script to run nsm tests: scripts/ci/run-nsm-tests.sh
  • add nsm integration tests: tests/nsm_tests.go

@hunchback
Copy link
Collaborator

@hunchback hunchback commented Mar 1, 2019

Regarding comments please see: https://blog.golang.org/godoc-documenting-go-code

Specifically the convention is to proceed the comment text with the name of function/method being documented.

@hunchback
Copy link
Collaborator

@hunchback hunchback commented Mar 1, 2019

Other than nsm.NewNsmProve() used by the analyzer ( https://github.com/skydive-project/skydive/pull/1636/files#diff-a990c838e9c8856f44e8e5edebf41b96 ) all other function/types should be private and start with a lowercase

@matrohon
Copy link
Contributor Author

@matrohon matrohon commented Mar 28, 2019

@hunchback thanks for your review. I've updated and rebased the Patch.

topology/probes/nsm/connection.go Outdated Show resolved Hide resolved
topology/probes/nsm/connection.go Outdated Show resolved Hide resolved
topology/probes/nsm/connection.go Outdated Show resolved Hide resolved
topology/probes/nsm/connection.go Show resolved Hide resolved
return
}
p.g.RemoveEventListener(p)
for _, conn := range p.nsmds {
Copy link
Collaborator

@safchain safchain May 3, 2019

Choose a reason for hiding this comment

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

p.nsmds should be protected by a mutex as it is read/write from two go-routine L104, L124 and here

Copy link
Contributor Author

@matrohon matrohon May 6, 2019

Choose a reason for hiding this comment

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

I'll use the mutex of the probe struct

topology/probes/nsm/nsm.go Outdated Show resolved Hide resolved
topology/probes/nsm/nsm.go Outdated Show resolved Hide resolved
@matrohon matrohon force-pushed the nsm-probe-only branch from 9ac130a to af8e41c May 8, 2019
@safchain
Copy link
Collaborator

@safchain safchain commented May 9, 2019

run skydive-scale-tests

@matrohon matrohon force-pushed the nsm-probe-only branch 4 times, most recently from 77a2a70 to 56496d1 May 15, 2019
matrohon added 4 commits May 17, 2019
This probe listens at NSM crossconnect monitoring events.
For each new crossconnect reported by nsm, a new Skydive edge is
created.

See: https://github.com/networkservicemesh/networkservicemesh

Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
matrohon added 20 commits May 17, 2019
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
Signed-off-by: Mathieu Rohon <mathieu.rohon@orange.com>
@lebauce lebauce merged commit d7997fc into skydive-project:master May 17, 2019
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants