Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

docs: Network picture refers to Clear instead of Kata #378

Closed
wants to merge 1 commit into from

Conversation

nitkon
Copy link
Contributor

@nitkon nitkon commented Feb 10, 2019

The network diagram refers to clear containers
bridge rather than kata containers bridge. Fix
it.

Fixes: #298

Signed-off-by: Nitesh Konkar [email protected]

@nitkon
Copy link
Contributor Author

nitkon commented Feb 10, 2019

/test

@jodh-intel
Copy link
Contributor

Hi @nitkon - thanks for doing this, but our rule wrt images is that we always include the "source" for the images too.

In this case, I see that due to an oversight, it appears there is no source for network.png.

@egernst - could you find that source file possibly?

@grahamwhaley
Copy link
Contributor

Yay for the fix.
Originally I think the docs/diagrams from CC were in a gdoc, and then made it into an odp (or the other way around). I think you can see that file here https://github.com/clearcontainers/runtime/tree/master/docs/architecture
but, generally as @jodh-intel says we try to use 'source based image formats', so if we can get that network diagram as an svg as well, yes please ;-)

@egernst
Copy link
Member

egernst commented Feb 20, 2019

Yikes! Good catch. @nitkon can you provide svg?

@nitkon nitkon force-pushed the master branch 5 times, most recently from 11d5388 to 1d2146f Compare February 20, 2019 11:40
@nitkon
Copy link
Contributor Author

nitkon commented Feb 20, 2019

@egernst: Sure. Updated my PR.

@jodh-intel
Copy link
Contributor

Thanks @nitkon - that svg looks like a representation of the png rather than a pure vector representation.

However, I guess unless @egernst can find the original, there isn't much we can do...?

@nitkon
Copy link
Contributor Author

nitkon commented Feb 20, 2019

@jodh-intel : Yes, I first edited the png and then converted to svg. I am not sure about what is the right flow.

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

Adding a DNM (and a request change) whilst I see if I can track down the original

@grahamwhaley
Copy link
Contributor

whilst looking around... this might tickle you #1

@grahamwhaley
Copy link
Contributor

OK, I tried the ODP file over at https://github.com/clearcontainers/runtime/tree/master/docs/architecture, but libreoffice told me that was corrupt (maybe a text/binary file commit issue?).
So, I searched back in time and found a copy over at https://github.com/intel/cc-oci-runtime/tree/master/documentation
I'll attach both that ODP, and the network diagram I exported using libreoffice, and can view in inkscape.
to export btw, I did a select-all on the relevant page and then an export with the 'selection' box ticked.

architecture-diagrams.odp.gz
net.svg.gz

@nitkon - can you edit the attached SVG? fingers crossed!

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

dropping my 'request for change' now we have the files avail - but leaving the WIP label until we have sorted it out.

@grahamwhaley
Copy link
Contributor

Heh heh - so.... we are getting there, but let's see if we can iron a couple of things out...

  • the png still says 'clear containers' (probably because @nitkon took my svg, which I had not updated ...)
  • the svg does not render on github :-( That is the same for me in fedora image viewer - you just get a blog of background hatching. I have a feeling it will be some strange background or bounding box in the svg source that libreoffice injected - hopefully we can figure out what, fix it, and document it for the future. I'm sure the file views OK in inkscape... but it would be nice if it viewed on github as well for instance.

@nitkon
Copy link
Contributor Author

nitkon commented Feb 20, 2019

@grahamwhaley : I edited your svg to have Kata instead of Clear which looked perfect in Inkscape but looks like github cannot render it.

@grahamwhaley
Copy link
Contributor

yeah. another oddity - when I look at the png file on this PR it shows me the one in the main tree - I'm wondering if that is because the PR seems to want to merge from your nitkon:master - but, that doesn't feel right. not sure if github has gone bonkers or..?

The network diagram refers to clear containers
bridge rather than kata containers bridge. Fix
it.

Fixes: kata-containers#298

Signed-off-by: Nitesh Konkar [email protected]
@jodh-intel
Copy link
Contributor

@egernst - could you verify this looks like the original?

@jodh-intel
Copy link
Contributor

Re-ping @egernst :)

@amshinde
Copy link
Member

@nitkon the png looke good to me, but the svg cannot be rendered like you mentioned above. Can you fix this so that the svg is rendered in github as well.

@raravena80
Copy link
Member

@egernst @nitkon ping. Thx!

@grahamwhaley
Copy link
Contributor

I think I'd like to close this @nitkon , for a number of reasons, none of them your fault :-) :

  • the PR is now conflicted as the file has since been moved into a subdir
  • that picture I suspect is just now out of date on how Kata networking works, particularly given the pending Topic/network simplify runtime#1214
  • nobody ever came back to ack that was the original picture/odp file to use

I'd like to hand this off to @amshinde and @mcastelino to combine with kata-containers/runtime#1214 and their cleanup of Kata networking - so, folks, when we clean up and deprecate the old ways (bridge), can we also go and try and fix, or archive, or remove, or mark as out of date, the architecture documentation as well please?

wdyt @nitkon ?

@nitkon
Copy link
Contributor Author

nitkon commented May 1, 2019

@grahamwhaley: Sounds reasonable to me. Should I close PR?

@amshinde
Copy link
Member

amshinde commented May 1, 2019

Yes I think this will no longer be valid with kata-containers/runtime#1214.
I will take a look at that PR next week to see it is merged.

@nitkon I think we can close this PR and handle updating the docs after the other PR is merged.

@nitkon nitkon closed this May 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Architecture document network picture refers to Clear Containers
8 participants